This is an archive of the discontinued LLVM Phabricator instance.

Fix PR21694 - Revert improper use of divide in SCEV HowFarToZero computation
ClosedPublic

Authored by meheff on Dec 4 2014, 7:15 PM.

Details

Reviewers
majnemer
atrick
Summary

r219517 added a use of SCEV divide in HowFarToZero computation. This divide can produce incorrect results as we are using an unsigned divide for what should be a modular divide. This change reverts back to a more conservative computation using trailing zeros.

Diff Detail

Event Timeline

meheff updated this revision to Diff 16977.Dec 4 2014, 7:15 PM
meheff retitled this revision from to Fix PR21694 - Revert improper use of divide in SCEV HowFarToZero computation.
meheff updated this object.
meheff edited the test plan for this revision. (Show Details)
meheff added reviewers: atrick, majnemer.
meheff added a subscriber: Unknown Object (MLST).
majnemer accepted this revision.Dec 4 2014, 10:24 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 4 2014, 10:24 PM
meheff added a comment.Dec 5 2014, 1:30 PM

Andrew, any comment on this? This just reverts back to a previous more
conservative (and correct) analysis.

Mark

meheff closed this revision.Dec 10 2014, 2:56 PM
atrick edited edge metadata.Dec 10 2014, 5:21 PM

Sorry I didn't respond. I wasn't able to easily convince myself that this fixes the problem. Since this reverts to the previous implementation, I think it's a good change. But the code is written in a way that is extremely hard to reason about.

For example:

  • StepV could be negative but still be a power of two, is this expected?
  • Why are we checking GetMinTrailingZeros(getNegativeSCEV(Start)) instead of GetMinTrailingZeros(Distance)?

If we can only handle counting up here, lets be more clear about that.