B3 should be able to compile Patchpoints with Register and Any constraints
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Nov 2015 20:41:06 +0000 (20:41 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Nov 2015 20:41:06 +0000 (20:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=151209

Reviewed by Geoffrey Garen.

Most of this patch is about adding tests, since Register constraints already worked but
were untested, and almost all forms of the Any constraint worked.

One change that I'm making is that Patchpoints now default to forCall effects rather
than no effects. I think this is better because if a client of Patchpoint never supplies
any effects, it's best to assume the worst.

My testing uncovered only one real bug: moveConstants() would turn all non-zero double
constants into Loads, so the optimization to fold constants into a Stackmap would stop
working. Instead of telling the Stackmap that the double value it wanted is actually a
constant, we would tell it that it's a register and it would emit code to materialize
the double into that register. I worked around this by having moveConstants() create
ConstDoubleValue's just for the Stackmaps, which lowerToAir() recognizes and does the
right thing with (i.e. folds into the stackmap instead of materializing).

* b3/B3LowerToAir.cpp:
(JSC::B3::Air::LowerToAir::fillStackmap):
(JSC::B3::Air::LowerToAir::lower):
* b3/B3MoveConstants.cpp:
* b3/B3PatchpointValue.cpp:
(JSC::B3::PatchpointValue::PatchpointValue):
* b3/testb3.cpp:
(JSC::B3::testPatchpointCallArg):
(JSC::B3::testPatchpointFixedRegister):
(JSC::B3::testPatchpointAny):
(JSC::B3::testPatchpointAnyImm):
(JSC::B3::testPatchpointManyImms):
(JSC::B3::testSimpleCheck):
(JSC::B3::run):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/b3/B3LowerToAir.cpp
Source/JavaScriptCore/b3/B3MoveConstants.cpp
Source/JavaScriptCore/b3/B3PatchpointValue.cpp
Source/JavaScriptCore/b3/testb3.cpp

index cb99992..89f59a9 100644 (file)
@@ -1,3 +1,40 @@
+2015-11-12  Filip Pizlo  <fpizlo@apple.com>
+
+        B3 should be able to compile Patchpoints with Register and Any constraints
+        https://bugs.webkit.org/show_bug.cgi?id=151209
+
+        Reviewed by Geoffrey Garen.
+
+        Most of this patch is about adding tests, since Register constraints already worked but
+        were untested, and almost all forms of the Any constraint worked.
+
+        One change that I'm making is that Patchpoints now default to forCall effects rather
+        than no effects. I think this is better because if a client of Patchpoint never supplies
+        any effects, it's best to assume the worst.
+
+        My testing uncovered only one real bug: moveConstants() would turn all non-zero double
+        constants into Loads, so the optimization to fold constants into a Stackmap would stop
+        working. Instead of telling the Stackmap that the double value it wanted is actually a
+        constant, we would tell it that it's a register and it would emit code to materialize
+        the double into that register. I worked around this by having moveConstants() create
+        ConstDoubleValue's just for the Stackmaps, which lowerToAir() recognizes and does the
+        right thing with (i.e. folds into the stackmap instead of materializing).
+
+        * b3/B3LowerToAir.cpp:
+        (JSC::B3::Air::LowerToAir::fillStackmap):
+        (JSC::B3::Air::LowerToAir::lower):
+        * b3/B3MoveConstants.cpp:
+        * b3/B3PatchpointValue.cpp:
+        (JSC::B3::PatchpointValue::PatchpointValue):
+        * b3/testb3.cpp:
+        (JSC::B3::testPatchpointCallArg):
+        (JSC::B3::testPatchpointFixedRegister):
+        (JSC::B3::testPatchpointAny):
+        (JSC::B3::testPatchpointAnyImm):
+        (JSC::B3::testPatchpointManyImms):
+        (JSC::B3::testSimpleCheck):
+        (JSC::B3::run):
+
 2015-11-12  Mark Lam  <mark.lam@apple.com>
 
         Move some prototypes to header files.
index e02b028..51076fb 100644 (file)
@@ -732,9 +732,10 @@ private:
                     arg = imm(value.value());
                 else if (value.value()->hasInt64())
                     arg = Arg::imm64(value.value()->asInt64());
-                else if (value.value()->hasDouble())
+                else if (value.value()->hasDouble() && canBeInternal(value.value())) {
+                    commitInternal(value.value());
                     arg = Arg::imm64(bitwise_cast<int64_t>(value.value()->asDouble()));
-                else
+                else
                     arg = tmp(value.value());
                 break;
             case ValueRep::SomeRegister:
@@ -1383,7 +1384,8 @@ private:
         }
 
         case ConstDouble: {
-            // We expect that the moveConstants() phase has run.
+            // We expect that the moveConstants() phase has run, and any doubles referenced from
+            // stackmaps get fused.
             RELEASE_ASSERT(isIdentical(m_value->asDouble(), 0.0));
             append(MoveZeroToDouble, tmp(m_value));
             return;
index 2ddd1a7..699b859 100644 (file)
@@ -82,11 +82,28 @@ public:
             
             for (unsigned valueIndex = 0; valueIndex < block->size(); ++valueIndex) {
                 Value* value = block->at(valueIndex);
-                for (Value*& child : value->children()) {
+                StackmapValue* stackmap = value->as<StackmapValue>();
+                for (unsigned childIndex = 0; childIndex < value->numChildren(); ++childIndex) {
+                    Value*& child = value->child(childIndex);
                     if (!needsMotion(child))
                         continue;
 
-                    child = materialize(valueIndex, child->key(), value->origin());
+                    ValueKey key = child->key();
+                    if (stackmap
+                        && goesInTable(key)
+                        && stackmap->constrainedChild(childIndex).rep() == ValueRep::Any) {
+                        // This is a weird special case. When we constant-fold an argument to a
+                        // stackmap, and that argument has the Any constraint, we want to just
+                        // tell the stackmap's generator that the argument is a constant rather
+                        // than materializing it in a register. For this to work, we need
+                        // lowerToAir to see this argument as a constant rather than as a load
+                        // from a table.
+                        child = m_insertionSet.insertValue(
+                            valueIndex, key.materialize(m_proc, value->origin()));
+                        continue;
+                    }
+                    
+                    child = materialize(valueIndex, key, value->origin());
                 }
             }
             
index 6345ee3..8dec1e9 100644 (file)
@@ -36,6 +36,7 @@ PatchpointValue::~PatchpointValue()
 
 PatchpointValue::PatchpointValue(unsigned index, Type type, Origin origin)
     : StackmapValue(index, CheckedOpcode, Patchpoint, type, origin)
+    , effects(Effects::forCall())
 {
 }
 
index 94847ae..6549bba 100644 (file)
@@ -2693,6 +2693,115 @@ void testPatchpointCallArg()
     CHECK(compileAndRun<int>(proc, 1, 2) == 3);
 }
 
+void testPatchpointFixedRegister()
+{
+    Procedure proc;
+    BasicBlock* root = proc.addBlock();
+    Value* arg1 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
+    Value* arg2 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1);
+    PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Int32, Origin());
+    patchpoint->append(ConstrainedValue(arg1, ValueRep(GPRInfo::regT0)));
+    patchpoint->append(ConstrainedValue(arg2, ValueRep(GPRInfo::regT1)));
+    patchpoint->setGenerator(
+        [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+            CHECK(params.reps.size() == 3);
+            CHECK(params.reps[0].isGPR());
+            CHECK(params.reps[1] == ValueRep(GPRInfo::regT0));
+            CHECK(params.reps[2] == ValueRep(GPRInfo::regT1));
+            GPRReg result = params.reps[0].gpr();
+
+            if (result == GPRInfo::regT1) {
+                jit.move(GPRInfo::regT1, result);
+                jit.add32(GPRInfo::regT0, result);
+            } else {
+                jit.move(GPRInfo::regT0, result);
+                jit.add32(GPRInfo::regT1, result);
+            }
+        });
+    root->appendNew<ControlValue>(proc, Return, Origin(), patchpoint);
+
+    CHECK(compileAndRun<int>(proc, 1, 2) == 3);
+}
+
+void testPatchpointAny()
+{
+    Procedure proc;
+    BasicBlock* root = proc.addBlock();
+    Value* arg1 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
+    Value* arg2 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1);
+    PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Int32, Origin());
+    patchpoint->append(ConstrainedValue(arg1, ValueRep::Any));
+    patchpoint->append(ConstrainedValue(arg2, ValueRep::Any));
+    patchpoint->setGenerator(
+        [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+            // We shouldn't have spilled the inputs, so we assert that they're in registers.
+            CHECK(params.reps.size() == 3);
+            CHECK(params.reps[0].isGPR());
+            CHECK(params.reps[1].isGPR());
+            CHECK(params.reps[2].isGPR());
+            jit.move(params.reps[1].gpr(), params.reps[0].gpr());
+            jit.add32(params.reps[2].gpr(), params.reps[0].gpr());
+        });
+    root->appendNew<ControlValue>(proc, Return, Origin(), patchpoint);
+
+    CHECK(compileAndRun<int>(proc, 1, 2) == 3);
+}
+
+void testPatchpointAnyImm()
+{
+    Procedure proc;
+    BasicBlock* root = proc.addBlock();
+    Value* arg1 = root->appendNew<Value>(
+        proc, Trunc, Origin(),
+        root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0));
+    Value* arg2 = root->appendNew<Const32Value>(proc, Origin(), 42);
+    PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Int32, Origin());
+    patchpoint->append(ConstrainedValue(arg1, ValueRep::Any));
+    patchpoint->append(ConstrainedValue(arg2, ValueRep::Any));
+    patchpoint->setGenerator(
+        [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+            CHECK(params.reps.size() == 3);
+            CHECK(params.reps[0].isGPR());
+            CHECK(params.reps[1].isGPR());
+            CHECK(params.reps[2].isConstant());
+            CHECK(params.reps[2].value() == 42);
+            jit.add32(
+                CCallHelpers::TrustedImm32(static_cast<int32_t>(params.reps[2].value())),
+                params.reps[1].gpr(), params.reps[0].gpr());
+        });
+    root->appendNew<ControlValue>(proc, Return, Origin(), patchpoint);
+
+    CHECK(compileAndRun<int>(proc, 1) == 43);
+}
+
+void testPatchpointManyImms()
+{
+    Procedure proc;
+    BasicBlock* root = proc.addBlock();
+    Value* arg1 = root->appendNew<Const32Value>(proc, Origin(), 42);
+    Value* arg2 = root->appendNew<Const64Value>(proc, Origin(), 43);
+    Value* arg3 = root->appendNew<Const64Value>(proc, Origin(), 43000000000000ll);
+    Value* arg4 = root->appendNew<ConstDoubleValue>(proc, Origin(), 42.5);
+    PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Void, Origin());
+    patchpoint->append(ConstrainedValue(arg1, ValueRep::Any));
+    patchpoint->append(ConstrainedValue(arg2, ValueRep::Any));
+    patchpoint->append(ConstrainedValue(arg3, ValueRep::Any));
+    patchpoint->append(ConstrainedValue(arg4, ValueRep::Any));
+    patchpoint->setGenerator(
+        [&] (CCallHelpers&, const StackmapGenerationParams& params) {
+            CHECK(params.reps.size() == 4);
+            CHECK(params.reps[0] == ValueRep::constant(42));
+            CHECK(params.reps[1] == ValueRep::constant(43));
+            CHECK(params.reps[2] == ValueRep::constant(43000000000000ll));
+            CHECK(params.reps[3] == ValueRep::constant(bitwise_cast<int64_t>(42.5)));
+        });
+    root->appendNew<ControlValue>(
+        proc, Return, Origin(),
+        root->appendNew<Const32Value>(proc, Origin(), 0));
+
+    CHECK(!compileAndRun<int>(proc));
+}
+
 void testSimpleCheck()
 {
     Procedure proc;
@@ -3841,6 +3950,10 @@ void run(const char* filter)
 
     RUN(testSimplePatchpoint());
     RUN(testPatchpointCallArg());
+    RUN(testPatchpointFixedRegister());
+    RUN(testPatchpointAny());
+    RUN(testPatchpointAnyImm());
+    RUN(testPatchpointManyImms());
     RUN(testSimpleCheck());
     RUN(testCheckLessThan());
     RUN(testCheckMegaCombo());