This is an archive of the discontinued LLVM Phabricator instance.

[SCEVExpander] Have hoistIVInc preserve LCSSA
ClosedPublic

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

Details

Summary

(Note: the problematic invocation of hoistIVInc that caused PR24804 came
from IndVarSimplify, not from SCEVExpander itself)

Fixes PR24804. Test case by David Majnemer.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 41354.Nov 29 2015, 2:29 PM
sanjoy retitled this revision from to [SCEVExpander] Have hoistIVInc preserve LCSSA.
sanjoy updated this object.
sanjoy added reviewers: hfinkel, majnemer.
sanjoy added a subscriber: llvm-commits.
mzolotukhin edited edge metadata.Dec 7 2015, 3:05 PM

Hi Sanjoy,

A couple of comments inline, otherwise the patch looks good to me.

Thanks,
Michael

lib/Analysis/ScalarEvolutionExpander.cpp
936–938 ↗(On Diff #41354)

With this change we should become more conservative in some cases; did you see any effect on the testsuite? E.g. how many tests would crash if we replace with condition with an assert?

lib/IR/Instruction.cpp
65–70 ↗(On Diff #41354)

This should probably go in a separate patch.

test/Transforms/IndVarSimplify/pr24804.ll
14–15 ↗(On Diff #41354)

Maybe run instnamer on the test?

sanjoy marked 2 inline comments as done.Dec 7 2015, 3:44 PM
sanjoy added inline comments.
lib/Analysis/ScalarEvolutionExpander.cpp
936–938 ↗(On Diff #41354)

Turning this into an assert does not fail any test in the current llvm test suite (i.e. the tests/ directory) except the newly added pr24804.ll. I haven't run this over any other IR corpus.

Having said that, I'm not too worried -- even in the "worst case", this will replace a correctness issue with a performance issue.

sanjoy updated this revision to Diff 42118.Dec 7 2015, 3:45 PM
sanjoy edited edge metadata.
  • address review

Having said that, I'm not too worried -- even in the "worst case", this will replace a correctness issue with a performance issue.

Yes, I agree. I actually meant LNT testsuite or any other real-world test-suite - I was just curious if it had any effect at all there.

This revision was automatically updated to reflect the committed changes.