This is an archive of the discontinued LLVM Phabricator instance.

[IRCE] Relax restriction on collected range checks
ClosedPublic

Authored by mkazantsev on Apr 6 2018, 4:25 AM.

Details

Summary

In IRCE, we have a very old legacy check that works when we collect comparisons that we
treat as range checks. It ensures that the value against which the indvar is compared is
loop invariant and is also positive.

This latter condition remained there since the times when IRCE was only able to handle
signed latch comparison. As the optimization evolved, it now learned how to intersect
signed or unsigned ranges, and this logic has no reliance on the fact that the right border
of each range should be positive.

The old implementation of this non-negativity check was also naive enough and just looked
into ranges (while most of other IRCE logic tries to use power of SCEV implications), so this
check did not allow to deal with the most simple case that looks like follows:

int size; // not known non-negative
int length; //known non-negative;
i = 0;
if (size != 0) {
  do {
    range_check(i < size);
    range_check(i < length);
  ++i;
  } while (i < size)
}

In this case, even if from some dominating conditions IRCE could parse loop
structure, it could only remove the range check against length and simply
ignored the check against size.

In this patch we remove this obsolete check. It will allow IRCE to pick comparison
against size as a potential range check and then let Range Intersection logic
decide whether it is OK to eliminate it or not.

Diff Detail

Repository
rL LLVM

Event Timeline

mkazantsev created this revision.Apr 6 2018, 4:25 AM
mkazantsev updated this revision to Diff 141312.Apr 6 2018, 4:33 AM

Added couple more tests.

mkazantsev updated this revision to Diff 141313.Apr 6 2018, 4:38 AM
samparker accepted this revision.Apr 6 2018, 5:16 AM

Hi Max,

LGTM with just one small nit, but its up to you.

I was playing with something very similar the week before last. For my example, I also needed to change the 'IsInductionVar' lamdba to use the NUW as well as NSW, is that valid? If so, I will get the patch up once this is in.

thanks.

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
316 ↗(On Diff #141313)

Do we really need a lambda for a single statement?

This revision is now accepted and ready to land.Apr 6 2018, 5:16 AM

Hi Sam,

Please make a patch and submit it to review, I don't have a good answer on top of my head but maybe you are right. We might still have some obsolete restrictions in this code that can now be lifted.

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
316 ↗(On Diff #141313)

Well, it's used twice, so let's leave it as is

This revision was automatically updated to reflect the committed changes.