Bug 152184

Summary: B3::reduceStrength should remove redundant Phi's
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 150279    
Attachments:
Description Flags
the patch benjamin: review+

Filip Pizlo
Reported 2015-12-11 13:24:03 PST
Patch forthcoming.
Attachments
the patch (6.38 KB, patch)
2015-12-11 13:32 PST, Filip Pizlo 		benjamin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2015-12-11 13:32:11 PST
Created attachment 267184 [details] the patch
Benjamin Poulain
Comment 2 2015-12-11 14:49:54 PST
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
Comment 3 2015-12-11 15:07:26 PST
(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
Comment 4 2015-12-11 16:18:08 PST
Landed in http://trac.webkit.org/changeset/193993
Note You need to log in before you can comment on or make changes to this bug.