[webkit-reviews] review denied: [Bug 196715] No longer make CheckInBounds a child of the things that rely on being dominated by it : [Attachment 377162] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 196715: No longer make CheckInBounds a child of the things that rely on
being dominated by it
https://bugs.webkit.org/show_bug.cgi?id=196715

Attachment 377162: Patch

https://bugs.webkit.org/attachment.cgi?id=377162&action=review




--- Comment #9 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 377162
  --> https://bugs.webkit.org/attachment.cgi?id=377162
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