Title: [220625] trunk
Revision
220625
Author
fpi...@apple.com
Date
2017-08-12 11:44:48 -0700 (Sat, 12 Aug 2017)

Log Message

Caging shouldn't have to use a patchpoint for adding
https://bugs.webkit.org/show_bug.cgi?id=175483

Reviewed by Mark Lam.
Source/_javascript_Core:


Caging involves doing a Add(ptr, largeConstant). All of B3's heuristics for how to deal with
constants and associative operations dictate that you always want to sink constants. For example,
Add(Add(a, constant), b) always becomes Add(Add(a, b), constant). This is profitable because in
typical code, it reveals downstream optimizations. But it's terrible in the case of caging, because
we want the large constant (which is shared by all caging operations) to be hoisted. Reassociating to
sink constants obscures the constant in this case. Currently, moveConstants is not smart enough to
reassociate, so instead of sinking largeConstant, it tries (and often fails) to sink some other
constants instead. Without some hacks, this is a 5% Kraken regression and a 1.6% Octane regression.
It's not clear that moveConstants could ever be smart enough to rematerialize that constant and then
hoist it - that would require quite a bit of algebraic reasoning. But the only case we know of where
our current constant reassociation heuristics are wrong is caging. So, we can get away with some
hacks for just stopping B3's reassociation only in this specific case.
        
Previously, we achieved this by concealing the Add(ptr, largeConstant) inside a patchpoint. That's
OK, but patchpoints are expensive. They require a SharedTask instance. They require callbacks from
the backend, including during register allocation. And they cannot be CSE'd. We do want B3 to know
that if we cage the same pointer in two places, both places will compute the same value.
        
This patch improves the situation by introducing the Opaque opcode. This is handled by LowerToAir as
if it was Identity, but all prior phases treat it as an unknown pure unary idempotent operation. I.e.
they know that Opaque(x) == Opaque(x) and that Opaque(Opaque(x)) == Opaque(x). But they don't know
that Opaque(x) == x until LowerToAir. So, you can use Opaque exactly when you know that B3 will mess
up your code but Air won't. (Currently we know of no cases where Air messes things up on a large
enough scale to warrant new opcodes.)
        
This change is perf-neutral, but may start to help as I add more uses of caged() in the FTL. It also
makes the code a bit less ugly.

* b3/B3LowerToAir.cpp:
(JSC::B3::Air::LowerToAir::shouldCopyPropagate):
(JSC::B3::Air::LowerToAir::lower):
* b3/B3Opcode.cpp:
(WTF::printInternal):
* b3/B3Opcode.h:
* b3/B3ReduceStrength.cpp:
* b3/B3Validate.cpp:
* b3/B3Value.cpp:
(JSC::B3::Value::effects const):
(JSC::B3::Value::key const):
(JSC::B3::Value::isFree const):
(JSC::B3::Value::typeFor):
* b3/B3Value.h:
* b3/B3ValueKey.cpp:
(JSC::B3::ValueKey::materialize const):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::caged):
* ftl/FTLOutput.cpp:
(JSC::FTL::Output::opaque):
* ftl/FTLOutput.h:

Websites/webkit.org:

        
Write documentation for the new Opaque opcode.

* docs/b3/intermediate-representation.html:

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (220624 => 220625)


--- trunk/Source/_javascript_Core/ChangeLog	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/ChangeLog	2017-08-12 18:44:48 UTC (rev 220625)
@@ -1,5 +1,62 @@
 2017-08-11  Filip Pizlo  <fpi...@apple.com>
 
+        Caging shouldn't have to use a patchpoint for adding
+        https://bugs.webkit.org/show_bug.cgi?id=175483
+
+        Reviewed by Mark Lam.
+
+        Caging involves doing a Add(ptr, largeConstant). All of B3's heuristics for how to deal with
+        constants and associative operations dictate that you always want to sink constants. For example,
+        Add(Add(a, constant), b) always becomes Add(Add(a, b), constant). This is profitable because in
+        typical code, it reveals downstream optimizations. But it's terrible in the case of caging, because
+        we want the large constant (which is shared by all caging operations) to be hoisted. Reassociating to
+        sink constants obscures the constant in this case. Currently, moveConstants is not smart enough to
+        reassociate, so instead of sinking largeConstant, it tries (and often fails) to sink some other
+        constants instead. Without some hacks, this is a 5% Kraken regression and a 1.6% Octane regression.
+        It's not clear that moveConstants could ever be smart enough to rematerialize that constant and then
+        hoist it - that would require quite a bit of algebraic reasoning. But the only case we know of where
+        our current constant reassociation heuristics are wrong is caging. So, we can get away with some
+        hacks for just stopping B3's reassociation only in this specific case.
+        
+        Previously, we achieved this by concealing the Add(ptr, largeConstant) inside a patchpoint. That's
+        OK, but patchpoints are expensive. They require a SharedTask instance. They require callbacks from
+        the backend, including during register allocation. And they cannot be CSE'd. We do want B3 to know
+        that if we cage the same pointer in two places, both places will compute the same value.
+        
+        This patch improves the situation by introducing the Opaque opcode. This is handled by LowerToAir as
+        if it was Identity, but all prior phases treat it as an unknown pure unary idempotent operation. I.e.
+        they know that Opaque(x) == Opaque(x) and that Opaque(Opaque(x)) == Opaque(x). But they don't know
+        that Opaque(x) == x until LowerToAir. So, you can use Opaque exactly when you know that B3 will mess
+        up your code but Air won't. (Currently we know of no cases where Air messes things up on a large
+        enough scale to warrant new opcodes.)
+        
+        This change is perf-neutral, but may start to help as I add more uses of caged() in the FTL. It also
+        makes the code a bit less ugly.
+
+        * b3/B3LowerToAir.cpp:
+        (JSC::B3::Air::LowerToAir::shouldCopyPropagate):
+        (JSC::B3::Air::LowerToAir::lower):
+        * b3/B3Opcode.cpp:
+        (WTF::printInternal):
+        * b3/B3Opcode.h:
+        * b3/B3ReduceStrength.cpp:
+        * b3/B3Validate.cpp:
+        * b3/B3Value.cpp:
+        (JSC::B3::Value::effects const):
+        (JSC::B3::Value::key const):
+        (JSC::B3::Value::isFree const):
+        (JSC::B3::Value::typeFor):
+        * b3/B3Value.h:
+        * b3/B3ValueKey.cpp:
+        (JSC::B3::ValueKey::materialize const):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::caged):
+        * ftl/FTLOutput.cpp:
+        (JSC::FTL::Output::opaque):
+        * ftl/FTLOutput.h:
+
+2017-08-11  Filip Pizlo  <fpi...@apple.com>
+
         ScopedArguments overflow storage needs to be in the JSValue gigacage
         https://bugs.webkit.org/show_bug.cgi?id=174923
 

Modified: trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp (220624 => 220625)


--- trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp	2017-08-12 18:44:48 UTC (rev 220625)
@@ -189,6 +189,7 @@
         switch (value->opcode()) {
         case Trunc:
         case Identity:
+        case Opaque:
             return true;
         default:
             return false;
@@ -3336,7 +3337,8 @@
             return;
         }
             
-        case Identity: {
+        case Identity:
+        case Opaque: {
             ASSERT(tmp(m_value->child(0)) == tmp(m_value));
             return;
         }

Modified: trunk/Source/_javascript_Core/b3/B3Opcode.cpp (220624 => 220625)


--- trunk/Source/_javascript_Core/b3/B3Opcode.cpp	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/b3/B3Opcode.cpp	2017-08-12 18:44:48 UTC (rev 220625)
@@ -106,6 +106,9 @@
     case Identity:
         out.print("Identity");
         return;
+    case Opaque:
+        out.print("Opaque");
+        return;
     case Const32:
         out.print("Const32");
         return;

Modified: trunk/Source/_javascript_Core/b3/B3Opcode.h (220624 => 220625)


--- trunk/Source/_javascript_Core/b3/B3Opcode.h	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/b3/B3Opcode.h	2017-08-12 18:44:48 UTC (rev 220625)
@@ -43,6 +43,11 @@
     
     // Polymorphic identity, usable with any value type.
     Identity,
+        
+    // This is an identity, but we prohibit the compiler from realizing this until the bitter end. This can
+    // be used to block reassociation and other compiler reasoning, if we find that it's wrong or
+    // unprofitable and we need an escape hatch.
+    Opaque,
 
     // Constants. Use the ConstValue* classes. Constants exist in the control flow, so that we can
     // reason about where we would construct them. Large constants are expensive to create.
@@ -258,7 +263,7 @@
     // of transformation or analysis that relies on the insight that Depend is really zero is unsound,
     // because it unlocks reordering of users of @result and @phantom.
     //
-    // On X86, this is lowered to a load-load fence and @result uses @phantom directly.
+    // On X86, this is lowered to a load-load fence and @result folds to zero.
     //
     // On ARM, this is lowered as if like:
     //

Modified: trunk/Source/_javascript_Core/b3/B3ReduceStrength.cpp (220624 => 220625)


--- trunk/Source/_javascript_Core/b3/B3ReduceStrength.cpp	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/b3/B3ReduceStrength.cpp	2017-08-12 18:44:48 UTC (rev 220625)
@@ -485,6 +485,15 @@
     void reduceValueStrength()
     {
         switch (m_value->opcode()) {
+        case Opaque:
+            // Turn this: Opaque(Opaque(value))
+            // Into this: Opaque(value)
+            if (m_value->child(0)->opcode() == Opaque) {
+                replaceWithIdentity(m_value->child(0));
+                break;
+            }
+            break;
+            
         case Add:
             handleCommutativity();
             

Modified: trunk/Source/_javascript_Core/b3/B3Validate.cpp (220624 => 220625)


--- trunk/Source/_javascript_Core/b3/B3Validate.cpp	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/b3/B3Validate.cpp	2017-08-12 18:44:48 UTC (rev 220625)
@@ -139,6 +139,7 @@
                 VALIDATE(value->type() == Void, ("At ", *value));
                 break;
             case Identity:
+            case Opaque:
                 VALIDATE(!value->kind().hasExtraBits(), ("At ", *value));
                 VALIDATE(value->numChildren() == 1, ("At ", *value));
                 VALIDATE(value->type() == value->child(0)->type(), ("At ", *value));

Modified: trunk/Source/_javascript_Core/b3/B3Value.cpp (220624 => 220625)


--- trunk/Source/_javascript_Core/b3/B3Value.cpp	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/b3/B3Value.cpp	2017-08-12 18:44:48 UTC (rev 220625)
@@ -546,6 +546,7 @@
     switch (opcode()) {
     case Nop:
     case Identity:
+    case Opaque:
     case Const32:
     case Const64:
     case ConstDouble:
@@ -700,10 +701,13 @@
 
 ValueKey Value::key() const
 {
+    // NOTE: Except for exotic things like CheckAdd and friends, we want every case here to have a
+    // corresponding case in ValueKey::materialize().
     switch (opcode()) {
     case FramePointer:
         return ValueKey(kind(), type());
     case Identity:
+    case Opaque:
     case Abs:
     case Ceil:
     case Floor:
@@ -794,6 +798,7 @@
     case ConstDouble:
     case ConstFloat:
     case Identity:
+    case Opaque:
     case Nop:
         return true;
     default:
@@ -809,6 +814,7 @@
 {
     switch (kind.opcode()) {
     case Identity:
+    case Opaque:
     case Add:
     case Sub:
     case Mul:

Modified: trunk/Source/_javascript_Core/b3/B3Value.h (220624 => 220625)


--- trunk/Source/_javascript_Core/b3/B3Value.h	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/b3/B3Value.h	2017-08-12 18:44:48 UTC (rev 220625)
@@ -328,6 +328,7 @@
                 badKind(kind, numArgs);
             break;
         case Identity:
+        case Opaque:
         case Neg:
         case Clz:
         case Abs:

Modified: trunk/Source/_javascript_Core/b3/B3ValueKey.cpp (220624 => 220625)


--- trunk/Source/_javascript_Core/b3/B3ValueKey.cpp	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/b3/B3ValueKey.cpp	2017-08-12 18:44:48 UTC (rev 220625)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -56,11 +56,20 @@
 
 Value* ValueKey::materialize(Procedure& proc, Origin origin) const
 {
+    // NOTE: We sometimes cannot return a Value* for some key, like for Check and friends. That's because
+    // though those nodes have side exit effects. It would be weird to materialize anything that has a side
+    // exit. We can't possibly know enough about a side exit to know where it would be safe to emit one.
     switch (opcode()) {
     case FramePointer:
         return proc.add<Value>(kind(), type(), origin);
     case Identity:
+    case Opaque:
+    case Abs:
+    case Floor:
+    case Ceil:
     case Sqrt:
+    case Neg:
+    case Depend:
     case SExt8:
     case SExt16:
     case SExt32:
@@ -71,7 +80,6 @@
     case IToF:
     case FloatToDouble:
     case DoubleToFloat:
-    case Check:
         return proc.add<Value>(kind(), type(), origin, child(proc, 0));
     case Add:
     case Sub:
@@ -96,6 +104,7 @@
     case Below:
     case AboveEqual:
     case BelowEqual:
+    case EqualOrUnordered:
         return proc.add<Value>(kind(), type(), origin, child(proc, 0), child(proc, 1));
     case Select:
         return proc.add<Value>(kind(), type(), origin, child(proc, 0), child(proc, 1), child(proc, 2));

Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (220624 => 220625)


--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp	2017-08-12 18:44:48 UTC (rev 220625)
@@ -11626,25 +11626,21 @@
         LValue basePtr = m_out.constIntPtr(Gigacage::basePtr(kind));
         LValue mask = m_out.constIntPtr(GIGACAGE_MASK);
         
-        // We don't have to worry about B3 messing up the bitAnd. Also, we want to get B3's excellent
-        // codegen for 2-operand andq on x86-64.
         LValue masked = m_out.bitAnd(ptr, mask);
-        
-        // But B3 will currently mess up the code generation of this add. Basically, any offset from what we
-        // compute here will get reassociated and folded with Gigacage::basePtr. There's a world in which
-        // moveConstants() observes that it needs to reassociate in order to hoist the big constants. But
-        // it's much easier to just block B3's badness here. That's what we do for now.
-        // FIXME: It would be better if we didn't have to do this hack.
-        // https://bugs.webkit.org/show_bug.cgi?id=175483
-        PatchpointValue* patchpoint = m_out.patchpoint(pointerType());
-        patchpoint->appendSomeRegister(basePtr);
-        patchpoint->appendSomeRegister(masked);
-        patchpoint->setGenerator(
-            [] (CCallHelpers& jit, const StackmapGenerationParams& params) {
-                jit.addPtr(params[1].gpr(), params[2].gpr(), params[0].gpr());
-            });
-        patchpoint->effects = Effects::none();
-        return patchpoint;
+        LValue result = m_out.add(masked, basePtr);
+
+        // Make sure that B3 doesn't try to do smart reassociation of these pointer bits.
+        // FIXME: In an ideal world, B3 would not do harmful reassociations, and if it did, it would be able
+        // to undo them during constant hoisting and regalloc. As it stands, if you remove this then Octane
+        // gets 1.6% slower and Kraken gets 5% slower. It's all because the basePtr, which is a constant,
+        // gets reassociated out of the add above and into the address arithmetic. This disables hoisting of
+        // the basePtr constant. Hoisting that constant is worth a lot more perf than the reassociation. One
+        // way to make this all work happily is to combine offset legalization with constant hoisting, and
+        // then teach it reassociation. So, Add(Add(a, b), const) where a is loop-invariant while b isn't
+        // will turn into Add(Add(a, const), b) by the constant hoister. We would have to teach B3 to do this
+        // and possibly other smart things if we want to be able to remove this opaque.
+        // https://bugs.webkit.org/show_bug.cgi?id=175493
+        return m_out.opaque(result);
     }
     
     void buildSwitch(SwitchData* data, LType type, LValue switchValue)

Modified: trunk/Source/_javascript_Core/ftl/FTLOutput.cpp (220624 => 220625)


--- trunk/Source/_javascript_Core/ftl/FTLOutput.cpp	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/ftl/FTLOutput.cpp	2017-08-12 18:44:48 UTC (rev 220625)
@@ -127,6 +127,11 @@
     return m_block->appendNew<B3::Value>(m_proc, B3::Phi, type, origin());
 }
 
+LValue Output::opaque(LValue value)
+{
+    return m_block->appendNew<Value>(m_proc, Opaque, origin(), value);
+}
+
 LValue Output::add(LValue left, LValue right)
 {
     if (Value* result = left->addConstant(m_proc, right)) {

Modified: trunk/Source/_javascript_Core/ftl/FTLOutput.h (220624 => 220625)


--- trunk/Source/_javascript_Core/ftl/FTLOutput.h	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Source/_javascript_Core/ftl/FTLOutput.h	2017-08-12 18:44:48 UTC (rev 220625)
@@ -151,6 +151,8 @@
     void addIncomingToPhi(LValue phi, ValueFromBlock);
     template<typename... Params>
     void addIncomingToPhi(LValue phi, ValueFromBlock, Params... theRest);
+    
+    LValue opaque(LValue);
 
     LValue add(LValue, LValue);
     LValue sub(LValue, LValue);

Modified: trunk/Websites/webkit.org/ChangeLog (220624 => 220625)


--- trunk/Websites/webkit.org/ChangeLog	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Websites/webkit.org/ChangeLog	2017-08-12 18:44:48 UTC (rev 220625)
@@ -1,3 +1,14 @@
+2017-08-11  Filip Pizlo  <fpi...@apple.com>
+
+        Caging shouldn't have to use a patchpoint for adding
+        https://bugs.webkit.org/show_bug.cgi?id=175483
+
+        Reviewed by Mark Lam.
+        
+        Write documentation for the new Opaque opcode.
+
+        * docs/b3/intermediate-representation.html:
+
 2017-08-07  Jon Davis  <j...@apple.com>
 
         Fixed superscript rendering for blog posts

Modified: trunk/Websites/webkit.org/docs/b3/intermediate-representation.html (220624 => 220625)


--- trunk/Websites/webkit.org/docs/b3/intermediate-representation.html	2017-08-12 18:40:07 UTC (rev 220624)
+++ trunk/Websites/webkit.org/docs/b3/intermediate-representation.html	2017-08-12 18:44:48 UTC (rev 220625)
@@ -216,6 +216,12 @@
         to an Identity by doing convertToIdentity(otherValue).  If the value is Void,
         convertToIdentity() converts it to a Nop instead.</dd>
 
+      <dt>T Opaque(T)</dt>
+      <dd>Returns the passed value.  May be used for any type except Void.  This opcode is provided as a hack
+        for avoiding B3 optimizations that the client finds unprofitable.  B3 treats Opaque as an identity
+        only during instruction selection.  All prior optimizations (including all B3 IR optimizations) do
+        not know what Opaque does, other than Opaque(x) == Opaque(x) and Opaque(Opaque(x)) == Opaque(x).</dd>
+
       <dt>Int32 Const32(constant)</dt>
       <dd>32-bit integer constant.  Must use the Const32Value class, which has space for the
         int32_t constant.</dd>
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to