This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Add smin/umin support to getRangeRef
ClosedPublic

Authored by reames on Sep 12 2019, 12:28 PM.

Details

Summary

We were failing to compute trip counts (both exact and maximum) for any loop which involved a comparison against either an umin or smin. It looks like this simply got missed when we added smin/umin to SCEV.

I'm more than a bit shocked that something this basis was missed for this long. Am I missing something?

Diff Detail

Event Timeline

reames created this revision.Sep 12 2019, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 12:28 PM
fhahn added a subscriber: fhahn.Sep 12 2019, 12:32 PM

I've put up a patch for SCEVUmin D67177. I plan to land it tomorrow.

reames updated this revision to Diff 219971.Sep 12 2019, 12:36 PM
reames edited the summary of this revision. (Show Details)

Rebase over precommited tests.

I've put up a patch for SCEVUmin D67177. I plan to land it tomorrow.

Ok, could you review this one then? The umin portion looks identical, so assume I remove that after rebasing on your change.

nikic accepted this revision.Sep 12 2019, 12:44 PM

LG. Note that umin/smin support is also missing from GetMinTrailingZerosImpl.

This revision is now accepted and ready to land.Sep 12 2019, 12:44 PM
nikic added inline comments.Sep 12 2019, 12:48 PM
lib/Analysis/ScalarEvolution.cpp
5602

Just a side note: For operations that have an intrinsic sign preference, I think it would be better to pass on that specific signedness as the SignHint (here signed, below unsigned). Not sure if there's an assumption about a getRangeRef() chain always using the same signedness somewhere though.

fhahn accepted this revision.Sep 12 2019, 1:07 PM

LGTM, thanks! I've just landed D67177, to avoid delays with this patch.

FYI my motivation for UMin support is improving SCEV results with information from loop guards: D67178.

This revision was automatically updated to reflect the committed changes.