Changeset 279107 in webkit

Timestamp:

Jun 22, 2021, 12:56:27 AM (5 years ago)

Author:

Angelos Oikonomopoulos

Message:

Properly set numFPRs on ARM with NEON/VFP_V3_D32
​https://bugs.webkit.org/show_bug.cgi?id=227212

Reviewed by Filip Pizlo.

Don't hardcode the number of FP regs on ARMv7 to 16; when targetting a
CPU with NEON or VFP_V3_d32, the number of FP regs is 32.

This also reverts the recent change to add an extra word to RegisterSet
which essentially covered up for this mismatch. The reason this bug only
manifested on certain compiler versions was that GCC 8.4/8.5 where built using
our buildroot infrastructure, whereas the other GCC versions we tested with
were debian system toolchains, targetting a lowest common denominator.

• assembler/MacroAssemblerARMv7.h:

(JSC::MacroAssemblerARMv7::std::initializer_list<int>):

• jit/RegisterSet.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• assembler/MacroAssemblerARMv7.h (modified) (2 diffs)
• jit/RegisterSet.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-06-22  Angelos Oikonomopoulos  <angelos@igalia.com>
+
+        Properly set numFPRs on ARM with NEON/VFP_V3_D32
+        https://bugs.webkit.org/show_bug.cgi?id=227212
+
+        Reviewed by Filip Pizlo.
+
+        Don't hardcode the number of FP regs on ARMv7 to 16; when targetting a
+        CPU with NEON or VFP_V3_d32, the number of FP regs is 32.
+
+        This also reverts the recent change to add an extra word to RegisterSet
+        which essentially covered up for this mismatch. The reason this bug only
+        manifested on certain compiler versions was that GCC 8.4/8.5 where built using
+        our buildroot infrastructure, whereas the other GCC versions we tested with
+        were debian system toolchains, targetting a lowest common denominator.
+
+        * assembler/MacroAssemblerARMv7.h:
+        (JSC::MacroAssemblerARMv7::std::initializer_list<int>):
+        * jit/RegisterSet.h:
+
 2021-06-21  Ross Kirsling  <ross.kirsling@sony.com>
 

trunk/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h

@@ -29 +29 @@
 #if ENABLE(ASSEMBLER)
 
+#include <initializer_list>
+
 #include "ARMv7Assembler.h"
 #include "AbstractMacroAssembler.h"
@@ -44 +46 @@
 
 public:
-    static constexpr unsigned numGPRs = 16;
-    static constexpr unsigned numFPRs = 16;
-    
+#define DUMMY_REGISTER_VALUE(id, name, r, cs) 0,
+    static constexpr unsigned numGPRs = std::initializer_list<int>({ FOR_EACH_GP_REGISTER(DUMMY_REGISTER_VALUE) }).size();
+    static constexpr unsigned numFPRs = std::initializer_list<int>({ FOR_EACH_FP_REGISTER(DUMMY_REGISTER_VALUE) }).size();
+#undef DUMMY_REGISTER_VALUE
     RegisterID scratchRegister() { return dataTempRegister; }
 

trunk/Source/JavaScriptCore/jit/RegisterSet.h

@@ -33 +33 @@
 #include "TempRegisterSet.h"
 #include <wtf/Bitmap.h>
-#include <wtf/Compiler.h>
 
 namespace JSC {
 
-#if CPU(ARM) && COMPILER(GCC)
-
-#if GCC_VERSION_AT_LEAST(8, 4, 0) && !GCC_VERSION_AT_LEAST(9, 0, 0)
-// GCC 8.4.0 and 8.5.0 seem to miscompile WTF:Bitmap::count code on
-// ARM, something that apparently was covered up by the extra
-// word. The issue seems to not manifest with GCC 8.3.0 and >9.
-// Temporarily cover up by adding back the + 1.
-#define REGISTERSET_BITMAP_SLACK 1
-#else
-#define REGISTERSET_BITMAP_SLACK 0
-#endif
-
-#else
-#define REGISTERSET_BITMAP_SLACK 0
-#endif
-
-typedef Bitmap<MacroAssembler::numGPRs + MacroAssembler::numFPRs + REGISTERSET_BITMAP_SLACK> RegisterBitmap;
+typedef Bitmap<MacroAssembler::numGPRs + MacroAssembler::numFPRs> RegisterBitmap;
 class RegisterAtOffsetList;