B3::reduceStrength should remove redundant Phi's

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 267184 [details]
the patch

[Benjamin Poulain]

Comment on attachment 267184 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267184&action=review

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1252
> +                if (otherChild == nullptr) {

if (!otherChild)

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1265
> +                // Wow, this would be super weird. It probably won't happen, except that things could
> +                // get weird as a consequence of stepwise simplifications in the strength reduction
> +                // fixpoint.
> +                continue;

We can only get here in two ways:
1) There are no values for this Phi
2) All the values are the Phi itself.

[2] would be the sign of an invalid SSA form.

[1] should have been eliminated by killDeadCode().

Wouldn't an ASSER_WITH_MESSAGE() make sense here?

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 267184 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267184&action=review
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1252
> > +                if (otherChild == nullptr) {
> 
> if (!otherChild)
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1265
> > +                // Wow, this would be super weird. It probably won't happen, except that things could
> > +                // get weird as a consequence of stepwise simplifications in the strength reduction
> > +                // fixpoint.
> > +                continue;
> 
> We can only get here in two ways:
> 1) There are no values for this Phi
> 2) All the values are the Phi itself.
> 
> [2] would be the sign of an invalid SSA form.
> 
> [1] should have been eliminated by killDeadCode().
> 
> Wouldn't an ASSER_WITH_MESSAGE() make sense here?

All of the values could be the Phi itself if this is a loop that became unreachable.  I don't see how it would happen, but in my experience all of these kinds of cases where we add an assertion even though we could have handled it with an early return mean that eventually we just remove the assertion.  Better to remove it now, so that if we change the ordering of the fixpoint, we don't have to worry about this assertion.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/193993