This is an archive of the discontinued LLVM Phabricator instance.

[IndVars] Have getInsertPointForUses preserve LCSSA
ClosedPublic

Authored by sanjoy on Nov 29 2015, 2:31 PM.

Details

Summary

Also add a stricter post-condition for IndVarSimplify.

Fixes PR25578. Test case by Michael Zolotukhin.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 41355.Nov 29 2015, 2:31 PM
sanjoy retitled this revision from to [IndVars] Have getInsertPointForUses preserve LCSSA.
sanjoy updated this object.
sanjoy added reviewers: hfinkel, mzolotukhin.
sanjoy added a subscriber: llvm-commits.
mzolotukhin accepted this revision.Dec 4 2015, 11:58 AM
mzolotukhin edited edge metadata.

Hi Sanjoy,

Thanks for fixing this, and sorry for the delayed response - I was on vacation. The patch looks good to me!

One question: are you certain that ind-vars was only breaking LCSSA for inner loops, and it's been always preserving LCSSA for other loops?

Michael

lib/Analysis/LoopInfo.cpp
203 ↗(On Diff #41355)

Could you add a comment for this function please?

This revision is now accepted and ready to land.Dec 4 2015, 11:58 AM

Hi Sanjoy,

Thanks for fixing this, and sorry for the delayed response - I was on vacation. The patch looks good to me!

One question: are you certain that ind-vars was only breaking LCSSA for inner loops, and it's been always preserving LCSSA for other loops?

No, I'm far from certain that IndVars preserves LCSSA for outer loops always. This is change just fixing the case that we have seen in practice. My guess / intuition is that we'll eventually have to move to a more principled and more obviously correct approach around preserving LCSSA in IndVars in the future.

Michael

lib/Analysis/LoopInfo.cpp
203 ↗(On Diff #41355)

There is already a comment on the header. I think we're moving away from repeating comments in both the .h and .cpp files.

sanjoy added a comment.Dec 7 2015, 1:44 PM

FYI: I'm waiting for an LGTM on D15058 before I check this in. Since this change adds a stronger assert (that -indvars preserves LCSSA on the entire loop nest), and D15058 is one known case where the assert fails, I'd rather check all of these in together.

Since this change adds a stronger assert (that -indvars preserves LCSSA on the entire loop nest), and D15058 is one known case where the assert fails, I'd rather check all of these in together.

That totally makes sense. Thanks for working on it!

Michael

This revision was automatically updated to reflect the committed changes.