[JSC] BranchAdd can override arguments of its stackmap
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Feb 2016 04:51:20 +0000 (04:51 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Feb 2016 04:51:20 +0000 (04:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154274

Patch by Benjamin Poulain <bpoulain@apple.com> on 2016-02-15
Reviewed by Filip Pizlo.

With the 3 operands BranchAdd added in r196513, we can run into
a register allocation such that the destination register is also
used by a value in the stack map.

It use to be that BranchAdd was a 2 operand instruction.
In that form, the destination is also one of the source and
can be recovered through Sub. There is no conflict between
destination and the stackmap.

After r196513, the destination has its own value. It is uncommon
on x86 because of the aggressive aliasing but that can happen.
On ARM, that's a standard form since there is no need for aliasing.

Since the arguments of the stackmap are of type EarlyUse,
they appeared as not interfering with the destination. When the register
allocator gives the same register to the destination and something in
the stack map, the result of BranchAdd destroys the value kept alive
for the stackmap.

In this patch, I introduce a concept very similar to ForceLateUse
to keep the argument of the stackmap live in CheckAdd. The new
role is "ForceLateUseUnlessRecoverable".

In this mode, anything that is not also an input argument becomes
LateUse. As such, it interferes with the destination of CheckAdd.
The arguments are recovered by the slow patch of CheckAdd. They
remain Early use.

This new modes ensure that destination can be aliased to the source
when that's useful, while making sure it is not aliased with another
value that needs to be live on exit.

* b3/B3CheckSpecial.cpp:
(JSC::B3::CheckSpecial::forEachArg):
* b3/B3LowerToAir.cpp:
(JSC::B3::Air::LowerToAir::lower):
* b3/B3PatchpointSpecial.cpp:
(JSC::B3::PatchpointSpecial::forEachArg):
* b3/B3StackmapSpecial.cpp:
(JSC::B3::StackmapSpecial::forEachArgImpl):
(WTF::printInternal):
* b3/B3StackmapSpecial.h:
* b3/B3StackmapValue.h:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/b3/B3CheckSpecial.cpp
Source/JavaScriptCore/b3/B3LowerToAir.cpp
Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp
Source/JavaScriptCore/b3/B3StackmapSpecial.cpp
Source/JavaScriptCore/b3/B3StackmapSpecial.h
Source/JavaScriptCore/b3/B3StackmapValue.h

index 60dd310..be3ce42 100644 (file)
@@ -1,3 +1,54 @@
+2016-02-15  Benjamin Poulain  <bpoulain@apple.com>
+
+        [JSC] BranchAdd can override arguments of its stackmap
+        https://bugs.webkit.org/show_bug.cgi?id=154274
+
+        Reviewed by Filip Pizlo.
+
+        With the 3 operands BranchAdd added in r196513, we can run into
+        a register allocation such that the destination register is also
+        used by a value in the stack map.
+
+        It use to be that BranchAdd was a 2 operand instruction.
+        In that form, the destination is also one of the source and
+        can be recovered through Sub. There is no conflict between
+        destination and the stackmap.
+
+        After r196513, the destination has its own value. It is uncommon
+        on x86 because of the aggressive aliasing but that can happen.
+        On ARM, that's a standard form since there is no need for aliasing.
+
+        Since the arguments of the stackmap are of type EarlyUse,
+        they appeared as not interfering with the destination. When the register
+        allocator gives the same register to the destination and something in
+        the stack map, the result of BranchAdd destroys the value kept alive
+        for the stackmap.
+
+        In this patch, I introduce a concept very similar to ForceLateUse
+        to keep the argument of the stackmap live in CheckAdd. The new
+        role is "ForceLateUseUnlessRecoverable".
+
+        In this mode, anything that is not also an input argument becomes
+        LateUse. As such, it interferes with the destination of CheckAdd.
+        The arguments are recovered by the slow patch of CheckAdd. They
+        remain Early use.
+
+        This new modes ensure that destination can be aliased to the source
+        when that's useful, while making sure it is not aliased with another
+        value that needs to be live on exit.
+
+        * b3/B3CheckSpecial.cpp:
+        (JSC::B3::CheckSpecial::forEachArg):
+        * b3/B3LowerToAir.cpp:
+        (JSC::B3::Air::LowerToAir::lower):
+        * b3/B3PatchpointSpecial.cpp:
+        (JSC::B3::PatchpointSpecial::forEachArg):
+        * b3/B3StackmapSpecial.cpp:
+        (JSC::B3::StackmapSpecial::forEachArgImpl):
+        (WTF::printInternal):
+        * b3/B3StackmapSpecial.h:
+        * b3/B3StackmapValue.h:
+
 2016-02-15  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Web Workers have no access to console for debugging
index ce5fb4c..2d48d43 100644 (file)
@@ -113,7 +113,11 @@ void CheckSpecial::forEachArg(Inst& inst, const ScopedLambda<Inst::EachArgCallba
             unsigned index = &arg - &hidden.args[0];
             callback(inst.args[1 + index], role, type, width);
         });
-    forEachArgImpl(numB3Args(inst), m_numCheckArgs + 1, inst, m_stackmapRole, callback);
+
+    Optional<unsigned> firstRecoverableIndex;
+    if (m_checkOpcode == BranchAdd32 || m_checkOpcode == BranchAdd64)
+        firstRecoverableIndex = 1;
+    forEachArgImpl(numB3Args(inst), m_numCheckArgs + 1, inst, m_stackmapRole, firstRecoverableIndex, callback);
 }
 
 bool CheckSpecial::isValid(Inst& inst)
index 6342c85..6944207 100644 (file)
@@ -2120,6 +2120,7 @@ private:
             switch (m_value->opcode()) {
             case CheckAdd:
                 opcode = opcodeForType(BranchAdd32, BranchAdd64, m_value->type());
+                stackmapRole = StackmapSpecial::ForceLateUseUnlessRecoverable;
                 commutativity = Commutative;
                 break;
             case CheckSub:
index 86b26d8..e2538d6 100644 (file)
@@ -51,7 +51,7 @@ void PatchpointSpecial::forEachArg(Inst& inst, const ScopedLambda<Inst::EachArgC
     if (inst.origin->type() != Void)
         callback(inst.args[argIndex++], Arg::Def, inst.origin->airType(), inst.origin->airWidth());
 
-    forEachArgImpl(0, argIndex, inst, SameAsRep, callback);
+    forEachArgImpl(0, argIndex, inst, SameAsRep, Nullopt, callback);
     argIndex += inst.origin->numChildren();
 
     for (unsigned i = inst.origin->as<PatchpointValue>()->numGPScratchRegisters; i--;)
index 5d554a7..72398b9 100644 (file)
@@ -73,7 +73,8 @@ const RegisterSet& StackmapSpecial::extraEarlyClobberedRegs(Inst& inst)
 
 void StackmapSpecial::forEachArgImpl(
     unsigned numIgnoredB3Args, unsigned numIgnoredAirArgs,
-    Inst& inst, RoleMode roleMode, const ScopedLambda<Inst::EachArgCallback>& callback)
+    Inst& inst, RoleMode roleMode, Optional<unsigned> firstRecoverableIndex,
+    const ScopedLambda<Inst::EachArgCallback>& callback)
 {
     StackmapValue* value = inst.origin->as<StackmapValue>();
     ASSERT(value);
@@ -89,6 +90,13 @@ void StackmapSpecial::forEachArgImpl(
 
         Arg::Role role;
         switch (roleMode) {
+        case ForceLateUseUnlessRecoverable:
+            ASSERT(firstRecoverableIndex);
+            if (arg != inst.args[*firstRecoverableIndex] && arg != inst.args[*firstRecoverableIndex + 1]) {
+                role = Arg::LateColdUse;
+                break;
+            }
+            FALLTHROUGH;
         case SameAsRep:
             switch (child.rep().kind()) {
             case ValueRep::WarmAny:
@@ -273,6 +281,9 @@ void printInternal(PrintStream& out, StackmapSpecial::RoleMode mode)
     case StackmapSpecial::SameAsRep:
         out.print("SameAsRep");
         return;
+    case StackmapSpecial::ForceLateUseUnlessRecoverable:
+        out.print("ForceLateUseUnlessRecoverable");
+        return;
     case StackmapSpecial::ForceLateUse:
         out.print("ForceLateUse");
         return;
index 8a37aee..9aa6137 100644 (file)
@@ -47,6 +47,7 @@ public:
 
     enum RoleMode : int8_t {
         SameAsRep,
+        ForceLateUseUnlessRecoverable,
         ForceLateUse
     };
 
@@ -59,7 +60,8 @@ protected:
     // subclasses that implement that.
     void forEachArgImpl(
         unsigned numIgnoredB3Args, unsigned numIgnoredAirArgs,
-        Air::Inst&, RoleMode, const ScopedLambda<Air::Inst::EachArgCallback>&);
+        Air::Inst&, RoleMode, Optional<unsigned> firstRecoverableIndex,
+        const ScopedLambda<Air::Inst::EachArgCallback>&);
     
     bool isValidImpl(
         unsigned numIgnoredB3Args, unsigned numIgnoredAirArgs,
index da59581..5ca61f1 100644 (file)
@@ -91,6 +91,11 @@ public:
     {
         appendVectorWithRep(vector, ValueRep::ColdAny);
     }
+    template<typename VectorType>
+    void appendLateColdAnys(const VectorType& vector)
+    {
+        appendVectorWithRep(vector, ValueRep::LateColdAny);
+    }
 
     // This is a helper for something you might do a lot of: append a value that should be constrained
     // to SomeRegister.