This is an archive of the discontinued LLVM Phabricator instance.

SCEVExpander::expandAddRecExprLiterally(): check before casting as Instruction
ClosedPublic

Authored by lebedev.ri on Jun 26 2018, 10:31 AM.

Details

Summary

An alternative to D48597.
Fixes PR37936.

The problem is as follows:

  1. indvars marks %dec as NUW.
  2. loop-instsimplify runs instsimplify, which constant-folds %dec to -1 (D47908)
  3. loop-reduce tries to do some further modification, but crashes with an type assertion in cast, because %dec is no longer an Instruction,

If the runline is split into two, i.e. you first run -indvars -loop-instsimplify,
store that into a file, and then run -loop-reduce, there is no crash.

So it looks like the problem is due to -loop-instsimplify not discarding SCEV.
But in this case we can just not crash if it's not an Instruction.
This is just a local fix, unlike D48597, so there may very well be other problems.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 26 2018, 10:31 AM
evstupac added inline comments.
lib/Analysis/ScalarEvolutionExpander.cpp
1172–1175 ↗(On Diff #152916)

It looks very similar to dyn_cast:

Instruction *TempIncV =
    dyn_cast<Instruction>(PN.getIncomingValueForBlock(LatchBlock));
if (!TempIncV)
  continue;
lebedev.ri marked an inline comment as done.

Simplify code via dyn_cast<>().

IMO this doesn't seem like an invalidation bug -- SCEV Expander needs to be smarter about dealing with these kinds of simplifications.

Does this sound better?

Yes -- that seems better to me.

BTW i think this is a wrong fix.
In this case, after -loop-reduce we still end up with the same output as the input, just slightly renamed, still with %dec.
So we can be doing -indvars -loop-instsimplify -loop-reduce any number of times, in an endless loop...

uabelho resigned from this revision.Jun 27 2018, 11:01 PM

This change indeed makes the crash go away but I have no idea if this is the right solution or if https://reviews.llvm.org/D48597 is better.

Frankly, I don't understand why we want it to be an instruction at all. Can these methods (i.e. hoistIVInc and isNormalAddRecExprPHI) be taught to work with values? For example, if it is not an instruction then we just consider it already hoisted.

mkazantsev added inline comments.Jun 28 2018, 2:52 AM
lib/Analysis/ScalarEvolutionExpander.cpp
1172 ↗(On Diff #152922)

Naming here is also confusing. V assumes that it should be a value. If there is no strong reason to keep it an instruction, I'd suggest reworking this code to deal with values.

lebedev.ri added inline comments.Jun 28 2018, 2:59 AM
lib/Analysis/ScalarEvolutionExpander.cpp
1172 ↗(On Diff #152922)

I'm afraid i have no knowledge of this code to do anything like that.
So the crash can either stay, or this band-aid can land, and a bug
about possibility to rework this code to work with Value be filled.

mkazantsev accepted this revision.Jun 28 2018, 8:17 PM

LGTM once a TODO comment is added.

lib/Analysis/ScalarEvolutionExpander.cpp
1172 ↗(On Diff #152922)

Ok, I'm actually OK with the current solution. Just add a TODO comment that this possibly can be reworked to avoid this cast at all.

This revision is now accepted and ready to land.Jun 28 2018, 8:17 PM
lebedev.ri marked 2 inline comments as done.

LGTM once a TODO comment is added.

Thank you for the review!

lebedev.ri added a comment.EditedJun 29 2018, 12:47 AM
This comment has been deleted.
lib/Analysis/ScalarEvolutionExpander.cpp
1172 ↗(On Diff #152922)

Ok, great, thank you.

This revision was automatically updated to reflect the committed changes.