This is an archive of the discontinued LLVM Phabricator instance.

[IRCE] Support narrow latch condition for wide range checks
ClosedPublic

Authored by mkazantsev on Jan 17 2019, 2:42 AM.

Details

Summary

This patch relaxes restrictions on types of latch condition and range check.
In current implementation, they should match. This patch allows to handle
wide range checks against narrow condition. The motivating example is the
following:

int N = ...
for (long i = 0; (int) i < N; i++) {
  if (i >= length) deopt;
}

Diff Detail

Repository
rL LLVM

Event Timeline

mkazantsev created this revision.Jan 17 2019, 2:42 AM
mkazantsev retitled this revision from [WIP][IRCE] Support narrow latch condition for wide range checks to [IRCE] Support narrow latch condition for wide range checks.Jan 20 2019, 10:28 PM
mkazantsev edited the summary of this revision. (Show Details)
mkazantsev added a subscriber: llvm-commits.

We've run 3 days of dedicated fuzz tests for that, no issues found.

reames requested changes to this revision.Jan 21 2019, 5:58 PM
reames added inline comments.
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
1071 ↗(On Diff #182234)

I can't convince myself this is correct when the IV would overflow. Can you explain why we don't need some form of overflow check here? Note that in LoopPredication in isSafeToTruncate, we *do* have an overflow check.

1398 ↗(On Diff #182234)

I can't justify this change to myself. Can you explain why this is correct?

This revision now requires changes to proceed.Jan 21 2019, 5:58 PM
mkazantsev marked an inline comment as done.Jan 22 2019, 12:19 AM
mkazantsev added inline comments.
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
1071 ↗(On Diff #182234)

We should not be doing IRCE if IV overflows its type. This is guaranteed by functions isSafeIncreasingBound, isSafeDecreasingBound. I will add some more tests to ensure that, but I'm pretty sure that we already have plenty.

mkazantsev marked an inline comment as done.Jan 22 2019, 12:52 AM
mkazantsev added inline comments.
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
1398 ↗(On Diff #182234)

Without this patch, the type of IndVarBase always matches with the type of range (and types of all range checks being affected). With this patch, we are dealing with something like:

for (i64 i = 0; trunc i64 i to i32 <s i32 n; i++) {
  if (i < i64 bound) deopt();
}

IVbase here is a i32 AddRec for trunc i64 i to i32, and range and SR are i64 ranges containing safe iteration bounds.

In this case, ExitMainLoopAt would be smin(sext i32 n to i64, i64 bound) which is basically a i64 value that doesn't exceed i32 max. We can trunc it to i32 and its value will not change. We would just make more truncations.

Basically, it doesn't matter. All values involved would also fit into the narrow type range. Using wider type has two benefits:

  • It gives us less truncs that are only needed for comparisons;
  • further transforms can completely get rid of the narrow type in the main loop (IVSimplify can then prove that sext(trunc(iv)) on latch check is a noop and we can use IV instead).
reames accepted this revision.Jan 22 2019, 7:44 PM

Please land w/flag disabled. Wait until bots cycle, then enable flag separately.

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
1071 ↗(On Diff #182234)

I followed through the code, questions answered.

This revision is now accepted and ready to land.Jan 22 2019, 7:44 PM
This revision was automatically updated to reflect the committed changes.