Changeset 165522 in webkit for trunk/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp

Timestamp:

Mar 12, 2014, 6:50:41 PM (11 years ago)

Author:

[email protected]

Message:

ASSERTION FAILED: node->op() == Phi

node->op() == SetArgument

​https://bugs.webkit.org/show_bug.cgi?id=130069

Reviewed by Geoffrey Garen.

This was a great assertion, and it represents our strictest interpretation of the rules of
our intermediate representation. However, fixing DCE to actually preserve the relevant
property would be hard, and it wouldn't have an observable effect right now because nobody
actually uses the propery of CPS that this assertion is checking for.

In particular, we do always require, and rely on, the fact that non-captured variables
have variablesAtTail refer to the last interesting use of the variable: a SetLocal if the
block assigns to the variable, a GetLocal if it only reads from it, and a Flush,
PhantomLocal, or Phi otherwise. We do preserve this property successfully and DCE was not
broken in this regard. But, in the strictest sense, CPS also means that for captured
variables, variablesAtTail also continues to point to the last relevant use of the
variable. In particular, if there are multiple GetLocals, then it should point to the last
one. This is hard for DCE to preserve. Also, nobody relies on variablesAtTail for captured
variables, except to check the VariableAccessData; but in that case, we don't really need
the *last* relevant use of the variable - any node that mentions the same variable will do
just fine.

So, this change loosens the assertion and adds a detailed FIXME describing what we would
have to do if we wanted to preserve the more strict property.

This also makes changes to various debug printing paths so that validation doesn't crash
during graph dump. This also adds tests for the interesting cases of DCE failing to
preserve CPS in the strictest sense. This also attempts to win the record for longest test
name.

• bytecode/CodeBlock.cpp:

(JSC::CodeBlock::hashAsStringIfPossible):
(JSC::CodeBlock::dumpAssumingJITType):

• bytecode/CodeBlock.h:
• bytecode/CodeOrigin.cpp:

(JSC::InlineCallFrame::hashAsStringIfPossible):
(JSC::InlineCallFrame::dumpBriefFunctionInformation):

• bytecode/CodeOrigin.h:
• dfg/DFGCPSRethreadingPhase.cpp:

(JSC::DFG::CPSRethreadingPhase::run):

• dfg/DFGDCEPhase.cpp:

(JSC::DFG::DCEPhase::cleanVariables):

• dfg/DFGInPlaceAbstractState.cpp:

(JSC::DFG::InPlaceAbstractState::mergeStateAtTail):

• runtime/FunctionExecutableDump.cpp:

(JSC::FunctionExecutableDump::dump):

• tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store-in-function-with-multiple-basic-blocks.js: Added.

(foo):

• tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store.js: Added.

(foo):

File:

• trunk/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -260 +260 @@
             if (node->op() == GetLocal) {
                 node = node->child1().node();
-                ASSERT(node->op() == Phi || node->op() == SetArgument);
+                
+                // FIXME: In the case that the variable is captured, we really want to be able
+                // to replace the variable-at-tail with the last use of the variable in the same
+                // way that CPS rethreading would do. The child of the GetLocal isn't necessarily
+                // the same as what CPS rethreading would do. For example, we may have:
+                //
+                // a: SetLocal(...) // live
+                // b: GetLocal(@a) // live
+                // c: GetLocal(@a) // dead
+                //
+                // When killing @c, the code below will set the variable-at-tail to @a, while CPS
+                // rethreading would have set @b. This is a benign bug, since all clients of CPS
+                // only use the variable-at-tail of captured variables to get the
+                // VariableAccessData and observe that it is in fact captured. But, this feels
+                // like it could cause bugs in the future.
+                //
+                // It's tempting to just dethread and then invoke CPS rethreading, but CPS
+                // rethreading fails to preserve exact ref-counts. So we would need a fixpoint.
+                // It's probably the case that this fixpoint will be guaranteed to converge after
+                // the second iteration (i.e. the second run of DCE will not kill anything and so
+                // will not need to dethread), but for now the safest approach is probably just to
+                // allow for this tiny bit of sloppiness.
+                //
+                // Another possible solution would be to simply say that DCE dethreads but then
+                // we never rethread before going to the backend. That feels intuitively right
+                // because it's unlikely that any of the phases after DCE in the backend rely on
+                // ThreadedCPS.
+                //
+                // https://bugs.webkit.org/show_bug.cgi?id=130115
+                ASSERT(
+                    node->op() == Phi || node->op() == SetArgument
+                    || node->variableAccessData()->isCaptured());
+                
                 if (node->shouldGenerate()) {
                     variables[i] = node;