[Webkit-unassigned] [Bug 55046] Share VFP detection for ARM in MacroAssemblerARMCommon class

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55046





--- Comment #11 from Gavin Barraclough <barraclough at apple.com>  2011-07-13 11:51:27 PST ---
So, first, apologies - my comments may be slightly confusing because I slightly mixed up what changes are in bug #55046, and what are in the patch on bug #55158.  But there is something that needs to change here.

There is a problem with this patch that I failed to clearly identify, which is that this is introducing dead, unused and untested code, and we don't like patched that do so.  You should really be changing the arm macro assemblers' supportsFloatingPoint() methods to use your new VFP detection mechanism within this patch.  (This change is currently in bug #55158.  Bug #55158 should continue to be separate and contain the changes to branchTruncateDoubleToInt32, and the removal of supportsFloatingPointTruncate, but the change of implementation of supportsFloatingPoint() should be in this patch).

> Hrm, I think I'm confused. s_isVFPPresent is the return value of isVFPPresent() already (and it's only called once to initialize it). So making isVFPPresent() be the value of s_isVFPPresent introduces a recursive dependency? :)

The problem here is that you are making all platforms have to read a global (a static member variable, s_isVFPPresent) to determine whether VFP is available.  Reading a static member is necessary on LINUX (which dynamically checks), but is not necessary on other platforms.

> Also I'm basically just moving code around, so I suppose that whatever issue exists would be already there?

No.  Currently supportsFloatingPoint returns an immediate value, you are changing this.

> In any case I didn't really understand the proposal. s_isVFPPresent is what's used in the MacroAssembler methods, and we use isVFPPresent() once to set the value. On Linux we use the /proc thing, and in the rest of platforms we just return a constant decided at compile time. But in any case it's only done once, unless I'm mistaken, and from there on s_isVFPPresent is used.

s_isVFPPresent will be set to a constant in one object file, but this fact will be invisible to other compilation units.


Let me try to clarify my proposal:

1) Rename your current 'isVFPPresent()' method to 'initializeIsVFPPresent()'.

2) Wrap all the 's_isVFPPresent' value and 'initializeIsVFPPresent()' (formerly 'isVFPPresent()') in #ifdef LINUX.

3) Add a new 'isVFPPresent()' method defined as follows:

#if OS(LINUX)
    static ALWAYS_INLINE bool isVFPPresent() { return s_isVFPPresent; }
#elif (COMPILER(RVCT) && defined(__TARGET_FPU_VFP)) || (COMPILER(GCC) && defined(__VFP_FP__))
    static ALWAYS_INLINE bool isVFPPresent() { return true; }
#else
    static ALWAYS_INLINE bool isVFPPresent() { return false; }
#endif

Then, rather than checking s_isVFPPresent directly, clients can call 'isVFPPresent()' to check for VFP (which will compile out to nothing on platforms other than LINUX).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.