Bug 196715 – No longer make CheckInBounds a child of the things that rely on being dominated by it

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: JavaScriptCore

[Saam Barati]

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.

[Justin Michaud]

Looks like we should revert https://trac.webkit.org/changeset/241228/webkit, move ssalowering and ignore CheckedGetStack for now.

[Justin Michaud]

Created attachment 377044 [details]
Patch

[Saam Barati]

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

[Justin Michaud]

Created attachment 377152 [details]
Patch

[EWS Watchlist]

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.

[Justin Michaud]

Created attachment 377162 [details]
Patch

[Justin Michaud]

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?

[EWS Watchlist]

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.

[Saam Barati]

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

[Justin Michaud]

Closing old bugs assigned to me

[Justin Michaud]

Closing old bugs assigned to me