Air::reportUsedRegisters must padInterference
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Mar 2019 22:57:08 +0000 (22:57 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Mar 2019 22:57:08 +0000 (22:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195303
<rdar://problem/48270343>

Reviewed by Keith Miller.

JSTests:

* stress/optional-def-arg-width-should-be-both-early-and-late-use.js: Added.

Source/JavaScriptCore:

reportUsedRegisters uses reg liveness to eliminate loads/moves into dead
registers. However, liveness can report incorrect results in certain
scenarios when considering liveness at instruction boundaries. For example,
it can go wrong when an Inst has a LateUse of a register and the following
Inst has an EarlyDef of that same register. Such a scenario could lead us
to incorrectly say the register is not live-in to the first Inst. Pad
interference inserts Nops between such instruction boundaries that cause
this issue.

The test with this patch fixes the issue in reportUsedRegisters. This patch
also conservatively makes it so that lowerAfterRegAlloc calls padInterference
since it also reasons about liveness.

* b3/air/AirLowerAfterRegAlloc.cpp:
(JSC::B3::Air::lowerAfterRegAlloc):
* b3/air/AirPadInterference.h:
* b3/air/AirReportUsedRegisters.cpp:
(JSC::B3::Air::reportUsedRegisters):
* b3/testb3.cpp:
(JSC::B3::testReportUsedRegistersLateUseNotDead):
(JSC::B3::run):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242569 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/b3/air/AirLowerAfterRegAlloc.cpp
Source/JavaScriptCore/b3/air/AirPadInterference.h
Source/JavaScriptCore/b3/air/AirReportUsedRegisters.cpp
Source/JavaScriptCore/b3/testb3.cpp

index 10cff83..007ecb6 100644 (file)
@@ -1,3 +1,13 @@
+2019-03-06  Saam Barati  <sbarati@apple.com>
+
+        Air::reportUsedRegisters must padInterference
+        https://bugs.webkit.org/show_bug.cgi?id=195303
+        <rdar://problem/48270343>
+
+        Reviewed by Keith Miller.
+
+        * stress/optional-def-arg-width-should-be-both-early-and-late-use.js: Added.
+
 2019-03-06  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] AI should not propagate AbstractValue relying on constant folding phase
diff --git a/JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js b/JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js
new file mode 100644 (file)
index 0000000..d21c412
--- /dev/null
@@ -0,0 +1,19 @@
+//@ runDefault("--useConcurrentJIT=false", "--thresholdForJITAfterWarmUp=10", "--thresholdForFTLOptimizeAfterWarmUp=1000")
+
+function v0(v1) {
+    function v7(v8) {
+        function v12(v13, v14) {
+            const v16 = v14 - -0x80000000;
+            const v19 = [13.37, 13.37, 13.37];
+            function v20() {
+                return v16;
+            }
+            return v19;
+        }
+        return v8(v12, v1);
+    }
+    const v27 = v7(v7);
+}
+for (let i = 0; i < 100; i++) {
+    v0(i);
+}
index abe4056..8e94272 100644 (file)
@@ -1,3 +1,33 @@
+2019-03-06  Saam Barati  <sbarati@apple.com>
+
+        Air::reportUsedRegisters must padInterference
+        https://bugs.webkit.org/show_bug.cgi?id=195303
+        <rdar://problem/48270343>
+
+        Reviewed by Keith Miller.
+
+        reportUsedRegisters uses reg liveness to eliminate loads/moves into dead
+        registers. However, liveness can report incorrect results in certain 
+        scenarios when considering liveness at instruction boundaries. For example,
+        it can go wrong when an Inst has a LateUse of a register and the following
+        Inst has an EarlyDef of that same register. Such a scenario could lead us
+        to incorrectly say the register is not live-in to the first Inst. Pad
+        interference inserts Nops between such instruction boundaries that cause
+        this issue.
+        
+        The test with this patch fixes the issue in reportUsedRegisters. This patch
+        also conservatively makes it so that lowerAfterRegAlloc calls padInterference
+        since it also reasons about liveness.
+
+        * b3/air/AirLowerAfterRegAlloc.cpp:
+        (JSC::B3::Air::lowerAfterRegAlloc):
+        * b3/air/AirPadInterference.h:
+        * b3/air/AirReportUsedRegisters.cpp:
+        (JSC::B3::Air::reportUsedRegisters):
+        * b3/testb3.cpp:
+        (JSC::B3::testReportUsedRegistersLateUseNotDead):
+        (JSC::B3::run):
+
 2019-03-06  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] AI should not propagate AbstractValue relying on constant folding phase
index bfef3b3..24c5c96 100644 (file)
@@ -34,6 +34,7 @@
 #include "AirEmitShuffle.h"
 #include "AirInsertionSet.h"
 #include "AirInstInlines.h"
+#include "AirPadInterference.h"
 #include "AirRegLiveness.h"
 #include "AirPhaseScope.h"
 #include "B3CCallValue.h"
@@ -77,6 +78,8 @@ void lowerAfterRegAlloc(Code& code)
     if (!haveAnyRelevant)
         return;
 
+    padInterference(code);
+
     HashMap<Inst*, RegisterSet> usedRegisters;
     
     RegLiveness liveness(code);
index 18f8083..ae6b72a 100644 (file)
@@ -32,11 +32,11 @@ namespace JSC { namespace B3 { namespace Air {
 class Code;
 
 // This isn't a phase - it's meant to be a utility that other phases use. Air reasons about liveness by
-// reasoning about interference at boundaries between instructions. This can go wrong - for example, a
+// reasoning about interference at boundaries between instructions. This is convenient because it works
+// great in the most common case: early uses and late defs. However, this can go wrong - for example, a
 // late use in one instruction doesn't actually interfere with an early def of the next instruction, but
-// Air thinks that it does. This is convenient because it works great in the most common case: early uses
-// and late defs. In practice, only the register allocators need to use this, since only they need to be
-// able to color the interference graph using a bounded number of colors.
+// Air thinks that it does. It can also go wrong by having liveness incorrectly report that something is
+// dead when it isn't.
 //
 // See https://bugs.webkit.org/show_bug.cgi?id=163548#c2 for more info.
 
index f08636b..b717aef 100644 (file)
@@ -31,6 +31,7 @@
 #include "AirArgInlines.h"
 #include "AirCode.h"
 #include "AirInstInlines.h"
+#include "AirPadInterference.h"
 #include "AirRegLiveness.h"
 #include "AirPhaseScope.h"
 
@@ -41,6 +42,8 @@ void reportUsedRegisters(Code& code)
     PhaseScope phaseScope(code, "reportUsedRegisters");
     
     static constexpr bool verbose = false;
+
+    padInterference(code);
     
     if (verbose)
         dataLog("Doing reportUsedRegisters on:\n", code);
index eb92812..2c75eb2 100644 (file)
@@ -16723,6 +16723,53 @@ void testDemotePatchpointTerminal()
     validate(proc);
 }
 
+void testReportUsedRegistersLateUseFollowedByEarlyDefDoesNotMarkUseAsDead()
+{
+    Procedure proc;
+    if (proc.optLevel() < 2)
+        return;
+    BasicBlock* root = proc.addBlock();
+
+    RegisterSet allRegs = RegisterSet::allGPRs();
+    allRegs.exclude(RegisterSet::stackRegisters());
+    allRegs.exclude(RegisterSet::reservedHardwareRegisters());
+
+    {
+        // Make every reg 42 (just needs to be a value other than 10).
+        PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Void, Origin());
+        Value* const42 = root->appendNew<Const32Value>(proc, Origin(), 42);
+        for (Reg reg : allRegs)
+            patchpoint->append(const42, ValueRep::reg(reg));
+        patchpoint->setGenerator([&] (CCallHelpers&, const StackmapGenerationParams&) { });
+    }
+
+    {
+        PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Void, Origin());
+        Value* const10 = root->appendNew<Const32Value>(proc, Origin(), 10);
+        for (Reg reg : allRegs)
+            patchpoint->append(const10, ValueRep::lateReg(reg));
+        patchpoint->setGenerator([&] (CCallHelpers& jit, const StackmapGenerationParams&) {
+            for (Reg reg : allRegs) {
+                auto done = jit.branch32(CCallHelpers::Equal, reg.gpr(), CCallHelpers::TrustedImm32(10));
+                jit.breakpoint();
+                done.link(&jit);
+            }
+        });
+    }
+
+    {
+        PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Int32, Origin());
+        patchpoint->resultConstraint = ValueRep::SomeEarlyRegister;
+        patchpoint->setGenerator([&] (CCallHelpers&, const StackmapGenerationParams& params) {
+            RELEASE_ASSERT(allRegs.contains(params[0].gpr()));
+        });
+    }
+
+    root->appendNewControlValue(proc, Return, Origin());
+
+    compileAndRun<void>(proc);
+}
+
 // Make sure the compiler does not try to optimize anything out.
 NEVER_INLINE double zero()
 {
@@ -18349,6 +18396,8 @@ void run(const char* filter)
         RUN(testTernarySubInstructionSelection(Trunc, Int32, Air::Sub32));
     }
 
+    RUN(testReportUsedRegistersLateUseFollowedByEarlyDefDoesNotMarkUseAsDead());
+
     if (tasks.isEmpty())
         usage();