This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Use both known bits and sign bits when computing range of SCEV unknowns
ClosedPublic

Authored by reames on Feb 11 2021, 12:01 PM.

Details

Summary

When computing a range for a SCEVUnknown, today we use computeKnownBits for unsigned ranges, and computeNumSignBots for signed ranges. This means we miss opportunities to improve range results.

One common missed pattern is that we have a signed range of a value which CKB can determine is positive, but CNSB doesn't convey that information. The current range includes the negative part, and is thus double the size.

Per the removed comment, the original concern which delayed using both (after some code merging years back) was a compile time concern. CTMark results (provided by Nikita, thanks!) showed a geomean impact of about 0.1%. This doesn't seem large enough to avoid higher quality results.

Diff Detail

Event Timeline

reames created this revision.Feb 11 2021, 12:01 PM
reames requested review of this revision.Feb 11 2021, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 12:01 PM
reames updated this revision to Diff 323193.Feb 11 2021, 5:48 PM
reames edited the summary of this revision. (Show Details)

Cleaned up patch for review.

nikic added a comment.Feb 12 2021, 2:04 AM

Could you share what your motivating test case for this was (if there is one)? I'm not opposed to this change (compile-time impact is minor, and should not have any pathological cases), but the test changes here don't look super compelling either. The main real improvement (where we're not just lining up signed and unsigned ranges) seems to be in increasing-or-decreasing-iv.ll, but the results there are still sub-optimal, and I think these can be made optimal by resolving the comment at https://github.com/llvm/llvm-project/blob/4348d8ab7f6aa0359d99e49c2498af4ba2767505/llvm/lib/Analysis/ScalarEvolution.cpp#L6096-L6097. I'm also wondering whether it would make sense to handle selects the same way as phis and combine the operand ranges.

Could you share what your motivating test case for this was (if there is one)? I'm not opposed to this change (compile-time impact is minor, and should not have any pathological cases), but the test changes here don't look super compelling either.

The motivating case which led me to stumble on this is an outer loop with a shift recurrence IV - specifically an ashr recurrence starting with a positive number - which is used to control the trip count of an inner loop. Without the change, we report a spurious signed range on the outer IV which includes a large negative portion.

In terms of transformation, the primary effect is blocking ashr to lshr conversion and LFTR in the outer loop. It also simply makes it much harder to tell what's going on when looking at the analysis output which is actually the direct motivation.

I'm also planning to further build on top of this FYI. In particular, we can use the trip count of the outer loop (small) and the fact we're looking at recurrence to get much tighter ranges. That's hard to do without this building block though.

The main real improvement (where we're not just lining up signed and unsigned ranges) seems to be in increasing-or-decreasing-iv.ll, but the results there are still sub-optimal, and I think these can be made optimal by resolving the comment at https://github.com/llvm/llvm-project/blob/4348d8ab7f6aa0359d99e49c2498af4ba2767505/llvm/lib/Analysis/ScalarEvolution.cpp#L6096-L6097. I'm also wondering whether it would make sense to handle selects the same way as phis and combine the operand ranges.

I haven't looked at the select side of things much. Your proposal seems to make sense on the surface, but this hasn't been relevant yet.

lebedev.ri accepted this revision.Feb 18 2021, 8:38 AM
lebedev.ri added a subscriber: lebedev.ri.

Looks good to me FWIW.

llvm/lib/Analysis/ScalarEvolution.cpp
5870

If computeKnownBits()-returned KnownBits claims to know one of the NS high (sign) bits,
would we get better results if instead of doing this here,
if we'd merge NS knowledge into Known, and repeated lines 5856-5858?

This revision is now accepted and ready to land.Feb 18 2021, 8:38 AM
reames added inline comments.Feb 18 2021, 8:58 AM
llvm/lib/Analysis/ScalarEvolution.cpp
5870

Yes actually. I'll do that in a follow up, thanks for the idea.