Changeset 225966 in webkit for trunk/Source/JavaScriptCore/dfg/DFGLICMPhase.cpp

Timestamp:

Dec 14, 2017, 10:20:07 PM (7 years ago)

Author:

[email protected]

Message:

The CleanUp after LICM is erroneously removing a Check
​https://bugs.webkit.org/show_bug.cgi?id=180852
<rdar://problem/36063494>

Reviewed by Filip Pizlo.

JSTests:

• stress/dont-run-cleanup-after-licm.js: Added.

Source/JavaScriptCore:

There was a bug where CleanUp phase relied on isProved() bits and LICM
changed them in an invalid way. The bug is as follows:

We have two loops, L1 and L2, and two preheaders, P1 and P2. L2 is nested
inside of L1. We have a Check inside a node inside L1, say in basic block BB,
and that Check dominates all of L2. This is also a hoisting candidate, so we
hoist it outside of L1 and put it inside P1. Then, when we run AI, we look at
the preheader for each loop inside L1, so P1 and P2. When considering P2,
we execute the Check. Inside P2, before any hoisting is done, this Check
is dead code, because BB dominates P2. When we use AI to "execute" the
Check, it'll set its proof status to proved. This is because inside P2,
in the program before LICM runs, the Check is indeed proven at P2. But
it is not proven inside P1. This "execute" call will set our proof status
for the node inside *P1*, hence, we crash.

The fix here is to make LICM precise when updating the ProofStatus of an edge.
It can trust the AI state at the preheader it hoists the node to, but it can't
trust the state when executing effects inside inner loops's preheaders.

• dfg/DFGPlan.cpp:

(JSC::DFG::Plan::compileInThreadImpl):

File:

• trunk/Source/JavaScriptCore/dfg/DFGLICMPhase.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/dfg/DFGLICMPhase.cpp

@@ -221 +221 @@
             }
         }
-        
+
         return changed;
     }
@@ -298 +298 @@
         }
 
-        // FIXME: We should adjust the Check: flags on the edges of node. There are phases that assume
-        // that those flags are correct even if AI is stale.
-        // https://bugs.webkit.org/show_bug.cgi?id=148544
         data.preHeader->insertBeforeTerminal(node);
         node->owner = data.preHeader;
@@ -307 +304 @@
         node->origin.wasHoisted |= addsBlindSpeculation;
         
+        // We can trust what AI proves about edge proof statuses when hoisting to the preheader.
+        m_state.trustEdgeProofs();
+        m_state.initializeTo(data.preHeader);
+        m_interpreter.execute(node);
+        // However, when walking various inner loops below, the proof status of
+        // an edge may be trivially true, even if it's not true in the preheader
+        // we hoist to. We don't allow the below node executions to change the
+        // state of edge proofs. An example of where a proof is trivially true
+        // is if we have two loops, L1 and L2, where L2 is nested inside L1. The
+        // header for L1 dominates L2. We hoist a Check from L1's header into L1's
+        // preheader. However, inside L2's preheader, we can't trust that AI will
+        // tell us this edge is proven. It's proven in L2's preheader because L2
+        // is dominated by L1's header. However, the edge is not guaranteed to be
+        // proven inside L1's preheader.
+        m_state.dontTrustEdgeProofs();
+
         // Modify the states at the end of the preHeader of the loop we hoisted to,
         // and all pre-headers inside the loop. This isn't a stability bottleneck right now
@@ -324 +337 @@
             if (!subPreHeader->cfaDidFinish)
                 continue;
+            // We handled this above.
+            if (subPreHeader == data.preHeader)
+                continue;
             m_state.initializeTo(subPreHeader);
             m_interpreter.execute(node);
         }
-        
+
         // It just so happens that all of the nodes we currently know how to hoist
         // don't have var-arg children. That may change and then we can fix this