This is an archive of the discontinued LLVM Phabricator instance.

[IndVarSimplify] Don't replace IV user with unsafe loop-invariant (PR45360)
ClosedPublic

Authored by lebedev.ri on Jun 18 2020, 10:06 AM.

Details

Summary

As PR45360 reports,
with new cost-model we can sometimes end up being able to expand udiv/urem instructions.
And that exposes at least one instance of when we do that
regardless of whether or not it is safe to do.
In this particular case, it's SimplifyIndvar::replaceIVUserWithLoopInvariant().

It seems to me, we simply need to check with isSafeToExpandAt() first.

The test isn't great. I'm not sure how to make it only run -indvars.

Fixes PR45360.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jun 18 2020, 10:06 AM
lebedev.ri marked an inline comment as done.
lebedev.ri edited the summary of this revision. (Show Details)

Actually, i guess we only need isSafeToExpand(), not whole isSafeToExpandAt().

Generally looks ok. Please use isSafeToExpandAt because the context can give us more information about divisor being non-zero.

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
660

This comment doesn't reflect what this method does, can you pls also fix it along, like, "Replace the UseInst with a loop invariant expression if it is safe"?

675

Please isSafeToExpandAt.

677

with non-speculable loop invariant maybe?

lebedev.ri marked 3 inline comments as done.
lebedev.ri edited the summary of this revision. (Show Details)

Thanks for taking a look!
Addressing review notes.

mkazantsev accepted this revision.Jun 22 2020, 11:09 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 22 2020, 11:09 PM

LGTM, thanks!

Thank you for the review!

This revision was automatically updated to reflect the committed changes.