Changeset 242569 in webkit

Timestamp:

Mar 6, 2019, 2:57:08 PM (7 years ago)

Author:

sbarati@apple.com

Message:

Air::reportUsedRegisters must padInterference
​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):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/b3/air/AirLowerAfterRegAlloc.cpp (modified) (2 diffs)
• Source/JavaScriptCore/b3/air/AirPadInterference.h (modified) (1 diff)
• Source/JavaScriptCore/b3/air/AirReportUsedRegisters.cpp (modified) (2 diffs)
• Source/JavaScriptCore/b3/testb3.cpp (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/b3/air/AirLowerAfterRegAlloc.cpp

@@ -35 +35 @@
 #include "AirInsertionSet.h"
 #include "AirInstInlines.h"
+#include "AirPadInterference.h"
 #include "AirRegLiveness.h"
 #include "AirPhaseScope.h"
@@ -77 +78 @@
     if (!haveAnyRelevant)
         return;
+
+    padInterference(code);
 
     HashMap<Inst*, RegisterSet> usedRegisters;

trunk/Source/JavaScriptCore/b3/air/AirPadInterference.h

@@ -33 +33 @@
 
 // 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.

trunk/Source/JavaScriptCore/b3/air/AirReportUsedRegisters.cpp

@@ -32 +32 @@
 #include "AirCode.h"
 #include "AirInstInlines.h"
+#include "AirPadInterference.h"
 #include "AirRegLiveness.h"
 #include "AirPhaseScope.h"
@@ -42 +43 @@
     
     static constexpr bool verbose = false;
+
+    padInterference(code);
     
     if (verbose)

trunk/Source/JavaScriptCore/b3/testb3.cpp

@@ -16722 +16722 @@
     demoteValues(proc, valuesToDemote);
     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);
 }
 
@@ -18350 +18397 @@
     }
 
+    RUN(testReportUsedRegistersLateUseFollowedByEarlyDefDoesNotMarkUseAsDead());
+
     if (tasks.isEmpty())
         usage();