- Revision
- 242866
- Author
- [email protected]
- Date
- 2019-03-13 02:24:56 -0700 (Wed, 13 Mar 2019)
Log Message
Merge r242569 - 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):
Modified Paths
Added Paths
Diff
Modified: releases/WebKitGTK/webkit-2.24/JSTests/ChangeLog (242865 => 242866)
--- releases/WebKitGTK/webkit-2.24/JSTests/ChangeLog 2019-03-13 09:24:50 UTC (rev 242865)
+++ releases/WebKitGTK/webkit-2.24/JSTests/ChangeLog 2019-03-13 09:24:56 UTC (rev 242866)
@@ -1,3 +1,13 @@
+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-02-28 Yusuke Suzuki <[email protected]>
[JSC] sizeof(JSString) should be 16
Added: releases/WebKitGTK/webkit-2.24/JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js (0 => 242866)
--- releases/WebKitGTK/webkit-2.24/JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js (rev 0)
+++ releases/WebKitGTK/webkit-2.24/JSTests/stress/optional-def-arg-width-should-be-both-early-and-late-use.js 2019-03-13 09:24:56 UTC (rev 242866)
@@ -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: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog (242865 => 242866)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog 2019-03-13 09:24:50 UTC (rev 242865)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog 2019-03-13 09:24:56 UTC (rev 242866)
@@ -1,3 +1,33 @@
+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-04 Carlos Garcia Campos <[email protected]>
[GLib] Returning G_TYPE_OBJECT from a constructor does not work
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirLowerAfterRegAlloc.cpp (242865 => 242866)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirLowerAfterRegAlloc.cpp 2019-03-13 09:24:50 UTC (rev 242865)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirLowerAfterRegAlloc.cpp 2019-03-13 09:24:56 UTC (rev 242866)
@@ -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: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirPadInterference.h (242865 => 242866)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirPadInterference.h 2019-03-13 09:24:50 UTC (rev 242865)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirPadInterference.h 2019-03-13 09:24:56 UTC (rev 242866)
@@ -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: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirReportUsedRegisters.cpp (242865 => 242866)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirReportUsedRegisters.cpp 2019-03-13 09:24:50 UTC (rev 242865)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/air/AirReportUsedRegisters.cpp 2019-03-13 09:24:56 UTC (rev 242866)
@@ -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: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/testb3.cpp (242865 => 242866)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/testb3.cpp 2019-03-13 09:24:50 UTC (rev 242865)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/b3/testb3.cpp 2019-03-13 09:24:56 UTC (rev 242866)
@@ -16562,6 +16562,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()
{
@@ -18188,6 +18235,8 @@
RUN(testTernarySubInstructionSelection(Trunc, Int32, Air::Sub32));
}
+ RUN(testReportUsedRegistersLateUseFollowedByEarlyDefDoesNotMarkUseAsDead());
+
if (tasks.isEmpty())
usage();
