ValueRep(DoubleRep(@v)) can not simply convert to @v
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 22 Jun 2017 23:34:05 +0000 (23:34 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 22 Jun 2017 23:34:05 +0000 (23:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173687
<rdar://problem/32855563>

Reviewed by Mark Lam.

JSTests:

* stress/dont-strength-reduce-valuerep-of-doublerep.js: Added.
(i.catch):

Source/JavaScriptCore:

Consider this IR:
 block#x
  p: Phi() // int32 and double flows into this phi from various control flow
  d: DoubleRep(@p)
  some uses of @d here
  v: ValueRep(DoubleRepUse:@d)
  a: NewArrayWithSize(Int32:@v)
  some more nodes here ...

Because the flow of ValueRep(DoubleRep(@p)) will not produce an Int32,
AI proves that the Int32 check will fail. Constant folding phase removes
all nodes after @a and inserts an Unreachable after the NewArrayWithSize node.

The IR then looks like this:
block#x
  p: Phi() // int32 and double flows into this phi from various control flow
  d: DoubleRep(@p)
  some uses of @d here
  v: ValueRep(DoubleRepUse:@d)
  a: NewArrayWithSize(Int32:@v)
  Unreachable

However, there was a strength reduction rule that tries eliminate redundant
conversions. It used to convert the program to:
block#x
  p: Phi() // int32 and double flows into this phi from various control flow
  d: DoubleRep(@p)
  some uses of @d here
  a: NewArrayWithSize(Int32:@p)
  Unreachable

However, at runtime, @p will actually be an Int32, so @a will not OSR exit,
and we'll crash. This patch removes this strength reduction rule since it
does not maintain what would have happened if we executed the program before
the rule.

This rule is also wrong for other types of programs (I'm not sure we'd
actually emit this code, but if such IR were generated, we would previously
optimize it incorrectly):
@a: Constant(JSTrue)
@b: DoubleRep(@a)
@c: ValueRep(@b)
@d: use(@c)

However, the strength reduction rule would've transformed this into:
@a: Constant(JSTrue)
@d: use(@a)

And this would be wrong because node @c before the transformation would
have produced the JSValue jsNumber(1.0).

This patch was neutral in the benchmark run I did.

* dfg/DFGStrengthReductionPhase.cpp:
(JSC::DFG::StrengthReductionPhase::handleNode):

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

JSTests/ChangeLog
JSTests/stress/dont-strength-reduce-valuerep-of-doublerep.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp

index 9140313..ebfe5f3 100644 (file)
@@ -1,3 +1,14 @@
+2017-06-22  Saam Barati  <sbarati@apple.com>
+
+        ValueRep(DoubleRep(@v)) can not simply convert to @v
+        https://bugs.webkit.org/show_bug.cgi?id=173687
+        <rdar://problem/32855563>
+
+        Reviewed by Mark Lam.
+
+        * stress/dont-strength-reduce-valuerep-of-doublerep.js: Added.
+        (i.catch):
+
 2017-06-22  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Object.values should be implemented in C++
diff --git a/JSTests/stress/dont-strength-reduce-valuerep-of-doublerep.js b/JSTests/stress/dont-strength-reduce-valuerep-of-doublerep.js
new file mode 100644 (file)
index 0000000..bb9783b
--- /dev/null
@@ -0,0 +1,15 @@
+let a2 = [];
+let thingy = {length: 2**55, __proto__: []};
+let func = (x) => x;
+
+noInline(Array.prototype.map);
+
+// This test should not crash.
+for (let i = 0; i < 100000; ++i) {
+    try {
+        if (i > 0 && (i % 1000) === 0)
+            thingy.map(func)
+        a2.map(func);
+    } catch(e) {
+    }
+}
index 2649cfe..5f9f8de 100644 (file)
@@ -1,3 +1,67 @@
+2017-06-22  Saam Barati  <sbarati@apple.com>
+
+        ValueRep(DoubleRep(@v)) can not simply convert to @v
+        https://bugs.webkit.org/show_bug.cgi?id=173687
+        <rdar://problem/32855563>
+
+        Reviewed by Mark Lam.
+
+        Consider this IR:
+         block#x
+          p: Phi() // int32 and double flows into this phi from various control flow
+          d: DoubleRep(@p)
+          some uses of @d here
+          v: ValueRep(DoubleRepUse:@d)
+          a: NewArrayWithSize(Int32:@v)
+          some more nodes here ...
+        
+        Because the flow of ValueRep(DoubleRep(@p)) will not produce an Int32,
+        AI proves that the Int32 check will fail. Constant folding phase removes
+        all nodes after @a and inserts an Unreachable after the NewArrayWithSize node.
+        
+        The IR then looks like this:
+        block#x
+          p: Phi() // int32 and double flows into this phi from various control flow
+          d: DoubleRep(@p)
+          some uses of @d here
+          v: ValueRep(DoubleRepUse:@d)
+          a: NewArrayWithSize(Int32:@v)
+          Unreachable
+        
+        However, there was a strength reduction rule that tries eliminate redundant
+        conversions. It used to convert the program to:
+        block#x
+          p: Phi() // int32 and double flows into this phi from various control flow
+          d: DoubleRep(@p)
+          some uses of @d here
+          a: NewArrayWithSize(Int32:@p)
+          Unreachable
+        
+        However, at runtime, @p will actually be an Int32, so @a will not OSR exit,
+        and we'll crash. This patch removes this strength reduction rule since it
+        does not maintain what would have happened if we executed the program before
+        the rule.
+        
+        This rule is also wrong for other types of programs (I'm not sure we'd
+        actually emit this code, but if such IR were generated, we would previously
+        optimize it incorrectly):
+        @a: Constant(JSTrue)
+        @b: DoubleRep(@a)
+        @c: ValueRep(@b)
+        @d: use(@c)
+        
+        However, the strength reduction rule would've transformed this into:
+        @a: Constant(JSTrue)
+        @d: use(@a)
+        
+        And this would be wrong because node @c before the transformation would
+        have produced the JSValue jsNumber(1.0).
+        
+        This patch was neutral in the benchmark run I did.
+
+        * dfg/DFGStrengthReductionPhase.cpp:
+        (JSC::DFG::StrengthReductionPhase::handleNode):
+
 2017-06-22  JF Bastien  <jfbastien@apple.com>
 
         ARM64: doubled executable memory limit from 32MiB to 64MiB
index 9fdf9ef..95ae19b 100644 (file)
@@ -215,11 +215,8 @@ private:
             break;
 
         case ValueRep:
-        case Int52Rep:
-        case DoubleRep: {
-            // This short-circuits circuitous conversions, like ValueRep(DoubleRep(value)) or
-            // even more complicated things. Like, it can handle a beast like
-            // ValueRep(DoubleRep(Int52Rep(value))).
+        case Int52Rep: {
+            // This short-circuits circuitous conversions, like ValueRep(Int52Rep(value)).
             
             // The only speculation that we would do beyond validating that we have a type that
             // can be represented a certain way is an Int32 check that would appear on Int52Rep
@@ -258,7 +255,6 @@ private:
                     hadInt32Check = true;
                     continue;
                     
-                case DoubleRep:
                 case ValueRep:
                     continue;