Diff
Modified: branches/safari-607-branch/JSTests/ChangeLog (242856 => 242857)
--- branches/safari-607-branch/JSTests/ChangeLog 2019-03-13 08:24:34 UTC (rev 242856)
+++ branches/safari-607-branch/JSTests/ChangeLog 2019-03-13 08:24:38 UTC (rev 242857)
@@ -1,5 +1,56 @@
2019-03-13 Babak Shafiei <[email protected]>
+ Cherry-pick r242569. rdar://problem/48839273
+
+ 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/_javascript_Core:
+
+ 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
+
+ 2019-03-06 Saam Barati <[email protected]>
+
+ 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-13 Babak Shafiei <[email protected]>
+
Cherry-pick r242276. rdar://problem/48839389
DFG: Loop-invariant code motion (LICM) should not hoist dead code
Added: branches/safari-607-branch/JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js (0 => 242857)
--- branches/safari-607-branch/JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js 2019-03-13 08:24:38 UTC (rev 242857)
@@ -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);
+}
Modified: branches/safari-607-branch/Source/_javascript_Core/ChangeLog (242856 => 242857)
--- branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-03-13 08:24:34 UTC (rev 242856)
+++ branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-03-13 08:24:38 UTC (rev 242857)
@@ -1,5 +1,76 @@
2019-03-13 Babak Shafiei <[email protected]>
+ Cherry-pick r242569. rdar://problem/48839273
+
+ 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/_javascript_Core:
+
+ 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
+
+ 2019-03-06 Saam Barati <[email protected]>
+
+ 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-13 Babak Shafiei <[email protected]>
+
Cherry-pick r242276. rdar://problem/48839389
DFG: Loop-invariant code motion (LICM) should not hoist dead code
Modified: branches/safari-607-branch/Source/_javascript_Core/b3/air/AirLowerAfterRegAlloc.cpp (242856 => 242857)
--- branches/safari-607-branch/Source/_javascript_Core/b3/air/AirLowerAfterRegAlloc.cpp 2019-03-13 08:24:34 UTC (rev 242856)
+++ branches/safari-607-branch/Source/_javascript_Core/b3/air/AirLowerAfterRegAlloc.cpp 2019-03-13 08:24:38 UTC (rev 242857)
@@ -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 @@
if (!haveAnyRelevant)
return;
+ padInterference(code);
+
HashMap<Inst*, RegisterSet> usedRegisters;
RegLiveness liveness(code);
Modified: branches/safari-607-branch/Source/_javascript_Core/b3/air/AirPadInterference.h (242856 => 242857)
--- branches/safari-607-branch/Source/_javascript_Core/b3/air/AirPadInterference.h 2019-03-13 08:24:34 UTC (rev 242856)
+++ branches/safari-607-branch/Source/_javascript_Core/b3/air/AirPadInterference.h 2019-03-13 08:24:38 UTC (rev 242857)
@@ -32,11 +32,11 @@
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.
Modified: branches/safari-607-branch/Source/_javascript_Core/b3/air/AirReportUsedRegisters.cpp (242856 => 242857)
--- branches/safari-607-branch/Source/_javascript_Core/b3/air/AirReportUsedRegisters.cpp 2019-03-13 08:24:34 UTC (rev 242856)
+++ branches/safari-607-branch/Source/_javascript_Core/b3/air/AirReportUsedRegisters.cpp 2019-03-13 08:24:38 UTC (rev 242857)
@@ -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 @@
PhaseScope phaseScope(code, "reportUsedRegisters");
static constexpr bool verbose = false;
+
+ padInterference(code);
if (verbose)
dataLog("Doing reportUsedRegisters on:\n", code);
Modified: branches/safari-607-branch/Source/_javascript_Core/b3/testb3.cpp (242856 => 242857)
--- branches/safari-607-branch/Source/_javascript_Core/b3/testb3.cpp 2019-03-13 08:24:34 UTC (rev 242856)
+++ branches/safari-607-branch/Source/_javascript_Core/b3/testb3.cpp 2019-03-13 08:24:38 UTC (rev 242857)
@@ -16280,6 +16280,53 @@
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()
{
@@ -17875,6 +17922,8 @@
RUN(testTernarySubInstructionSelection(Trunc, Int32, Air::Sub32));
}
+ RUN(testReportUsedRegistersLateUseFollowedByEarlyDefDoesNotMarkUseAsDead());
+
if (tasks.isEmpty())
usage();
