I have a couple of patches coming up that use VFP in ARMv7 for certain operations. I need exactly the same code present in ARM traditional to detect VFP, so after consulting on IRC we decided it made sense to share this in a common class as done in X86Common.
Created attachment 83483 [details] armcommon.diff I think I'll need a hand getting the Apple build foo right...
Attachment 83483 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/assembler/MacroAssemblerARMCommon.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/assembler/MacroAssemblerARMCommon.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/assembler/MacroAssemblerARMCommon.cpp:39: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/assembler/MacroAssemblerARMCommon.cpp:40: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 83673 [details] armcommon.diff Fix style issues (carried over from MacroAssemblerARM.cpp)
Comment on attachment 83673 [details] armcommon.diff r=me
Comment on attachment 83673 [details] armcommon.diff Won't build on Mac.
This is missing the XCode build bits, I'll get back to it.
Created attachment 91582 [details] armcommon.diff Add missing XCode build bits.
Created attachment 100105 [details] armcommon.diff Updated for ToT. Any chance to land this before it rots guys? :)
Comment on attachment 100105 [details] armcommon.diff Hi xan, this patch is going to add unnecessary dynamic checks on GGC/__VFP_FP__ and RVCT/__TARGET_FPU_VFP platforms, so I'd like to suggest the following: 1) remove s_isVFPPresent for non-LINUX platforms. 2) define isVFPPresent() for non-LINUX platforms as ALWAYS_INLINE & returning a constant true/false, so that these checks can be compile out. 3) for linux only, define isVFPPresent() to return the value of s_isVFPPresent. Hope this sounds good.
(In reply to comment #9) > (From update of attachment 100105 [details]) > Hi xan, this patch is going to add unnecessary dynamic checks on GGC/__VFP_FP__ and RVCT/__TARGET_FPU_VFP platforms, so I'd like to suggest the following: > > 1) remove s_isVFPPresent for non-LINUX platforms. > > 2) define isVFPPresent() for non-LINUX platforms as ALWAYS_INLINE & returning a constant true/false, so that these checks can be compile out. > > 3) for linux only, define isVFPPresent() to return the value of s_isVFPPresent. > > > Hope this sounds good. 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? :) Also I'm basically just moving code around, so I suppose that whatever issue exists would be already there? 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.
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).
Alright. I still had to read your comment twice, but I think I see the issue now. Indeed the confusing part was that you were mostly talking about something that is done in the other bug, but made the comment here :) About the dead code, I guess you are right, but since the patch also changes the ARM classic port I figured it was enough proof that it didn't break anything. In any case, I'll do these changes ASAP, thanks for the review.
Created attachment 100891 [details] Patch
OK, a few things. The JSC/ARMv7 was broken (as usual when I try to update some patch :S), so I had to fix some things: - One is bug #64571. - The other I still haven't managed to figure out, but the jsc shell fails to link: CXXLD Programs/jsc-1 ./.libs/libjavascriptcoregtk-1.0.so: undefined reference to `JSC::JIT::JIT(JSC::JSGlobalData*, JSC::CodeBlock*, void*)' collect2: ld returned 1 exit status But the ctor is not defined with a third parameter, so I'm not sure of what's going on. In any case because of this I can only verify that the patch compiles, but things might still blow up. Should be enough to say if it looks ok until I verify that it works though. Also, I didn't add anything from bug #55158, because the changes in the MacroAssemblerARMv7 require the rest of the patch to be correct (otherwise we'd try to execute the VFP path without VFP support?). Hope it makes sense.
Comment on attachment 100891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100891&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARMCommon.cpp:30 > +#if ENABLE(ASSEMBLER) && (CPU(ARM_TRADITIONAL) || CPU(ARM_THUMB2)) CPU(ARM) will be better to test both instruction sets > Source/JavaScriptCore/assembler/MacroAssemblerARMCommon.h:47 > + static ALWAYS_INLINE bool isVFPPresent(); The isVFPPresent should be in the protected section, not in the private one. > Source/JavaScriptCore/assembler/MacroAssemblerARMCommon.h:52 > +#if OS(LINUX) > +protected: > + static const bool s_isVFPPresent; > +#endif I think this looks better: protected: #if OS(LINUX) static const bool s_isVFPPresent; #endif
Comment on attachment 100891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100891&action=review Please fix Gabor's comments. I also do think it would much more sense for this patch to change MacroAssemblerARMv7's supportsFloatingPoint() method to call isVFPPresent(), rather than just returning true. Your patch is logically designed to affect ARM & ARMv7, but is unnecessarily only being tested on ARM. If you are not going to use this on ARMv7, then then code should just remain in MacroAssemblerARM.cpp. If you are going to use it on ARMv7, then do so! > Source/JavaScriptCore/assembler/MacroAssemblerARMCommon.cpp:67 > +{ Please move this to the header, we want this to be visible across all compilation units.
(In reply to comment #16) > (From update of attachment 100891 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100891&action=review > > Please fix Gabor's comments. > I also do think it would much more sense for this patch to change MacroAssemblerARMv7's supportsFloatingPoint() method to call isVFPPresent(), rather than just returning true. Your patch is logically designed to affect ARM & ARMv7, but is unnecessarily only being tested on ARM. If you are not going to use this on ARMv7, then then code should just remain in MacroAssemblerARM.cpp. If you are going to use it on ARMv7, then do so! I think for this patch probably the best idea is to not change the superclass of MacroAssemblerARMv7, and only do it later (bug #55158) when we start to use that functionality. As commented before changing supportsFloatingPoint() in ARMv7 without pulling the rest of the functionality would not make sense IMHO. In any case first I need to figure out why I cannot link JSC on ARMv7, otherwise I can only test the patchs for compilation errors. I'll get to that as soon as I have some time.
> I think for this patch probably the best idea is to not change the superclass of MacroAssemblerARMv7, and only do it later (bug #55158) when we start to use that functionality. As commented before changing supportsFloatingPoint() in ARMv7 without pulling the rest of the functionality would not make sense IMHO. Fine. I disagree (I think a patch that changes how ARMv7 FP detecting for existing supported FP works should be kept separate from a patch that enables support for a new operation), but these are small enough patches that whether the code goes in one or the other isn't a big enough deal to pick a fight over, we can do it your way. :-)
(In reply to comment #18) > Fine. I disagree (I think a patch that changes how ARMv7 FP detecting for existing supported FP works should be kept separate from a patch that enables support for a new operation), but these are small enough patches that whether the code goes in one or the other isn't a big enough deal to pick a fight over, we can do it your way. :-) OK, Jesus. I was mixing up steps. It makes sense to change supportsFloatingPoint at this point, because it's not yet merged with the Truncate thing. In my mind we were activating the truncate support in ARMv7 before we actually landed the code that in bug #55158, that's what didn't make sense. So I can do nothing at all in ARMv7 (as I said initially), or do what you suggest and change the hardcoded true for the new VFP detection method. Since you prefer the latter I'll just do that. Sorry for the noise.
I prefer the latter (change ARMv7 to use new mechanism in this patch), but can go either way. The only bit I strongly care about is moving the function that should be inlined into the header.