Page MenuHomePhabricator

[IRCE] Only check for NSW on equality predicates

Authored by samparker on Apr 13 2018, 4:33 AM.



After investigation discussed in D45439, it would seem that the nsw flag restriction is unnecessary in most cases. So the IsInductionVar lambda has been removed, the functionality extracted, and now only require nsw when using eq/ne predicates.

Diff Detail


Event Timeline

samparker created this revision.Apr 13 2018, 4:33 AM
mkazantsev added inline comments.Apr 16 2018, 1:47 AM
933 ↗(On Diff #142378)

How about non-constant steps?

For me it looks fine, but please let me run some tests with this patch. I don't see obvious problem, and if they aren't revealed by this testing, I'll give my approval. It will take few hours.

933 ↗(On Diff #142378)

Never mind, it's checked above. Could have been a dyn_cast on line 923.

mkazantsev added inline comments.Apr 16 2018, 1:59 AM
221 ↗(On Diff #142378)

Will you please add a check that the postloop is generated? It looks like we should have one here, am I correct?

mkazantsev added inline comments.Apr 16 2018, 2:05 AM
923 ↗(On Diff #142378)

Please do affinity check first and take StepRecurrence only if it passes, it will save you some compile time on non-affine phis. Also can be dyn_cast.

mkazantsev accepted this revision.Apr 16 2018, 6:36 AM

My tasting has passed. LGTM with minor changes (comments inline).

This revision is now accepted and ready to land.Apr 16 2018, 6:36 AM
samparker marked 2 inline comments as done.Apr 18 2018, 6:52 AM
samparker added inline comments.
221 ↗(On Diff #142378)

sure. you're correct.

Closed by commit rL330256: [IRCE] Only check for NSW on equality predicates (authored by sam_parker, committed by ). · Explain WhyApr 18 2018, 6:53 AM
This revision was automatically updated to reflect the committed changes.
mkazantsev added inline comments.May 4 2018, 12:40 AM

Hi Sam, there is a bug here. dyn_cast may return nullptr and then we still access it, which is UB. I've fixed it in Please review post-commit if you can.