Merge r230485 - REGRESSION(r227341 and r227742): AI and clobberize sh…

…ould be precise and consistent about the effectfulness of CompareEq https://bugs.webkit.org/show_bug.cgi?id=184455 Reviewed by Michael Saboff. LICM is sort of an assertion that AI is as precise as clobberize about effects. If clobberize says that something is not effectful, then LICM will try to hoist it. But LICM's AI hack (AtTailAbstractState) cannot handle hoisting of things that have effects. So, if AI thinks that the thing being hoisted does have effects, then we get a crash. In r227341, we incorrectly told AI that CompareEq(Untyped:, _) is effectful. In fact, only ComapreEq(Untyped:, Untyped:) is effectful, and clobberize knew this already. As a result, LICM would blow up if we hoisted CompareEq(Untyped:, Other:), which clobberize knew wasn't effectful. Instead of fixing this by making AI precise, in r227742 we made matters worse by then breaking clobberize to also think that CompareEq(Untyped:, _) is effectful. This fixes the whole situation by teaching both clobberize and AI that the only effectful form of CompareEq is ComapreEq(Untyped:, Untyped:). * dfg/DFGAbstractInterpreterInlines.h: (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): * dfg/DFGClobberize.h: (JSC::DFG::clobberize):
WebKit · May 7, 2018 · 66b5804 · 66b5804
1 parent 04bfdec
commit 66b5804
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 8 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,31 @@
+2018-04-10  Filip Pizlo  <fpizlo@apple.com>
+
+        REGRESSION(r227341 and r227742): AI and clobberize should be precise and consistent about the effectfulness of CompareEq
+        https://bugs.webkit.org/show_bug.cgi?id=184455
+
+        Reviewed by Michael Saboff.
+
+        LICM is sort of an assertion that AI is as precise as clobberize about effects. If clobberize
+        says that something is not effectful, then LICM will try to hoist it. But LICM's AI hack
+        (AtTailAbstractState) cannot handle hoisting of things that have effects. So, if AI thinks that
+        the thing being hoisted does have effects, then we get a crash.
+
+        In r227341, we incorrectly told AI that CompareEq(Untyped:, _) is effectful. In fact, only
+        ComapreEq(Untyped:, Untyped:) is effectful, and clobberize knew this already. As a result, LICM
+        would blow up if we hoisted CompareEq(Untyped:, Other:), which clobberize knew wasn't
+        effectful.
+
+        Instead of fixing this by making AI precise, in r227742 we made matters worse by then breaking
+        clobberize to also think that CompareEq(Untyped:, _) is effectful.
+
+        This fixes the whole situation by teaching both clobberize and AI that the only effectful form
+        of CompareEq is ComapreEq(Untyped:, Untyped:).
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+
 2018-04-04  Filip Pizlo  <fpizlo@apple.com>
 
         REGRESSION(r222563): removed DoubleReal type check causes tons of crashes because CSE has never known how to handle SaneChain

diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
@@ -1636,7 +1636,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
             }
         }
 
-        if (node->child1().useKind() == UntypedUse || node->child2().useKind() == UntypedUse)
+        if (node->isBinaryUseKind(UntypedUse))
             clobberWorld(node->origin.semantic, clobberLimit);
         forNode(node).setType(SpecBoolean);
         break;

diff --git a/Source/JavaScriptCore/dfg/DFGClobberize.h b/Source/JavaScriptCore/dfg/DFGClobberize.h
@@ -1557,13 +1557,8 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             write(HeapObjectCount);
             return;
         }
-
-        if (node->op() == CompareEq && node->isBinaryUseKind(ObjectUse)) {
-            def(PureValue(node));
-            return;
-        }
-        if (node->child1().useKind() == UntypedUse || node->child1().useKind() == ObjectUse
-            || node->child2().useKind() == UntypedUse || node->child2().useKind() == ObjectUse) {
+
+        if (node->isBinaryUseKind(UntypedUse)) {
             read(World);
             write(Heap);
             return;