Changeset 196616 in webkit

Timestamp:

Feb 15, 2016 8:51:20 PM (8 years ago)

Author:

commit-queue@webkit.org

Message:

[JSC] BranchAdd can override arguments of its stackmap
​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:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• b3/B3CheckSpecial.cpp (modified) (1 diff)
• b3/B3LowerToAir.cpp (modified) (1 diff)
• b3/B3PatchpointSpecial.cpp (modified) (1 diff)
• b3/B3StackmapSpecial.cpp (modified) (3 diffs)
• b3/B3StackmapSpecial.h (modified) (2 diffs)
• b3/B3StackmapValue.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

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

trunk/Source/JavaScriptCore/b3/B3CheckSpecial.cpp

@@ -114 +114 @@
             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);
 }
 

trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp

@@ -2121 +2121 @@
             case CheckAdd:
                 opcode = opcodeForType(BranchAdd32, BranchAdd64, m_value->type());
+                stackmapRole = StackmapSpecial::ForceLateUseUnlessRecoverable;
                 commutativity = Commutative;
                 break;

trunk/Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp

@@ -52 +52 @@
         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();
 

trunk/Source/JavaScriptCore/b3/B3StackmapSpecial.cpp

@@ -74 +74 @@
 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>();
@@ -90 +91 @@
         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()) {
@@ -274 +282 @@
         out.print("SameAsRep");
         return;
+    case StackmapSpecial::ForceLateUseUnlessRecoverable:
+        out.print("ForceLateUseUnlessRecoverable");
+        return;
     case StackmapSpecial::ForceLateUse:
         out.print("ForceLateUse");

trunk/Source/JavaScriptCore/b3/B3StackmapSpecial.h

@@ -48 +48 @@
     enum RoleMode : int8_t {
         SameAsRep,
+        ForceLateUseUnlessRecoverable,
         ForceLateUse
     };
@@ -60 +61 @@
     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(

trunk/Source/JavaScriptCore/b3/B3StackmapValue.h

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