Instead, we can use SSALowering to help us here. We can run SSALowering after LICM. We can also make this work with GetStack by having some kind of CheckedGetStack that gets lowered in SSALowering.
Looks like we should revert https://trac.webkit.org/changeset/241228/webkit, move ssalowering and ignore CheckedGetStack for now.
Created attachment 377044 [details] Patch
Comment on attachment 377044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377044&action=review > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:828 > result = insertionSet.insertNode( > - nodeIndex, node->prediction(), GetStack, node->origin, OpInfo(data), Edge(check, UntypedUse)); > + nodeIndex, node->prediction(), GetStack, node->origin, OpInfo(data)); Oh, this is what I meant by GetStack. Is this right? > Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:39 > +template <bool LowerNonEffectful> this should start with a lower case letter
Created attachment 377152 [details] Patch
Attachment 377152 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:240: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 377162 [details] Patch
Comment on attachment 377162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377162&action=review > Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:479 > + node->child2() = Edge(); This will remove the check. Can you make sure this is fine?
Attachment 377162 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:240: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 377162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377162&action=review > Source/JavaScriptCore/ChangeLog:9 > + Move ssa lowering of non-effectful nodes to after licm, this way we can never hoist a node above its check. > + We also add a new CheckedGetStack node that gets lowered in ssa lowering, for the same reason. You should actually explain this in some detail. I think examples will help understand what's going on. > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:-419 > + m_insertionSet.insertNode( > indexInBlock, SpecNone, CheckInBounds, node->origin, > node->child2(), Edge(length, Int32Use)); > node->convertToGetStack(data); > - node->child1() = Edge(check, UntypedUse); This needs to be CheckedGetStack. It's identical to code in the arguments elimination phase > Source/JavaScriptCore/dfg/DFGPlan.cpp:411 > + // We can safely lower anything could not be hoisted. Should be: "We can safely lower anything which will never be hoisted" maybe even add a "like PutByVal" > Source/JavaScriptCore/dfg/DFGPlan.cpp:450 > + RUN_PHASE(performSSALowering); probably the really nice way to do this is ensure that a GetByVal is never "safe to execute" after this lowering takes place. Not sure it's worth doing, but that's how we typically convey if it's ok to run one node in a different part oof the program > Source/JavaScriptCore/dfg/DFGPlan.cpp:452 > + // Clean up after ssa lowering for integer range optimization. Comment not needed >> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:479 >> + node->child2() = Edge(); > > This will remove the check. Can you make sure this is fine? it's not fine > Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:39 > +template <bool lowerNonEffectful> it's simpler to follow this if there were not a negative and the conditions inside here were inverted. > Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:83 > + if (lowerNonEffectful) > + lowerBoundsCheck(m_graph.child(m_node, 0), m_graph.child(m_node, 1), m_graph.child(m_node, 2)); this is wrong. All the atomics are effectful. Only HasIndexedProperty shouldn't be effectful. You can verify what I'm saying by looking in Clobberize
Closing old bugs assigned to me