This is an archive of the discontinued LLVM Phabricator instance.

LoopVectorize/iv-select-cmp: add tests for truncated IV
ClosedPublic

Authored by artagnon on Jul 24 2023, 7:14 AM.

Details

Summary

The current tests in iv-select-cmp.ll are not representative of clang
output of common real-world C programs, which are often written with i32
induction vars, as opposed to i64 induction vars. Hence, add five tests
corresponding to the following programs:

int test(int *a, int n) {
  int rdx = 331;
  for (int i = 0; i < n; i++) {
    if (a[i] > 3)
      rdx = i;
  }
  return rdx;
}

int test(int *a) {
  int rdx = 331;
  for (int i = 0; i < 20000; i++) {
    if (a[i] > 3)
      rdx = i;
 }
  return rdx;
}

int test(int *a, long n) {
  int rdx = 331;
  for (int i = 0; i < n; i++) {
    if (a[i] > 3)
      rdx = i;
  }
  return rdx;
}

int test(int *a, unsigned n) {
  int rdx = 331;
  for (int i = 0; i < n; i++) {
    if (a[i] > 3)
      rdx = i;
  }
  return rdx;
}

int test(int *a) {
  int rdx = 331;
  for (long i = INT_MIN - 1; i < UINT_MAX; i++) {
    if (a[i] > 3)
      rdx = i;
  }
  return rdx;
}

The first two can theoretically be vectorized without a runtime-check,
while the third and fourth cannot. The fifth cannot be vectorized, even
with a runtime-check.

This issue was found while reviewing D150851.

Diff Detail

Event Timeline

artagnon created this revision.Jul 24 2023, 7:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 7:14 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
artagnon requested review of this revision.Jul 24 2023, 7:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 7:14 AM
Mel-Chen added inline comments.Jul 24 2023, 11:51 PM
llvm/test/Transforms/LoopVectorize/iv-select-cmp.ll
71

nit: define i32 @select_icmp_const_truncated_iv(ptr nocapture readonly %a, i32 %n) {

artagnon updated this revision to Diff 543969.Jul 25 2023, 7:38 AM

Address review comments by Mel.

artagnon marked an inline comment as done.Jul 25 2023, 7:38 AM

Based on my response https://reviews.llvm.org/D150851#4531155, do you have any thoughts?
If you choose option 3, this patch can be landed.
If you choose option 1, this case can be cleaned up into three blocks: entry, for.body, and exit.
If you choose option 2, then this test case is not needed.

fhahn added inline comments.Jul 26 2023, 6:44 AM
llvm/test/Transforms/LoopVectorize/iv-select-cmp.ll
77

Is this needed?

80

nit: pass as i64?

fhahn added a comment.Jul 26 2023, 6:50 AM

Based on my response https://reviews.llvm.org/D150851#4531155, do you have any thoughts?
If you choose option 3, this patch can be landed.
If you choose option 1, this case can be cleaned up into three blocks: entry, for.body, and exit.
If you choose option 2, then this test case is not needed.

Regardless of the outcome, we should still have test coverage those cases to make sure whatever solution we end up with has test coverage for those cases.

llvm/test/Transforms/LoopVectorize/iv-select-cmp.ll
77

I guess it's needed reading back the latest comments for D150851. Would be good to add a comment with an explanation and also add negative version of the test (i.e. where %n isn't guaranteed to be signed positive in i32).

artagnon added inline comments.Jul 26 2023, 7:51 AM
llvm/test/Transforms/LoopVectorize/iv-select-cmp.ll
77

Just to be clear: this test doesn't pass with Mel's patch. I can add another test with icmp eq, but we can theoretically vectorize it without a runtime check, because of the nuw nsw flags on the add instruction corresponding to %inc. I think the comment with the explanations should be added by the final version of Mel's patch.

artagnon updated this revision to Diff 544375.Jul 26 2023, 8:10 AM

Address Florian's review comments.

artagnon retitled this revision from LoopVectorize/iv-select-cmp: add test for wide trip count (NFC) to LoopVectorize/iv-select-cmp: add tests for truncated IV.Jul 26 2023, 8:12 AM
artagnon edited the summary of this revision. (Show Details)
artagnon marked 3 inline comments as done.
artagnon updated this revision to Diff 544379.Jul 26 2023, 8:14 AM

Preserve newline at end of file.

artagnon updated this revision to Diff 549870.Aug 14 2023, 4:13 AM

Thoroughly cover all cases, effectively redoing patch.

artagnon edited the summary of this revision. (Show Details)Aug 14 2023, 4:17 AM

Hi Mel,

I've implemented (3) carefully, which I will post as a follow-up. Could you kindly double-check that the first two examples should be vectorized, while the other three shouldn't be?

Thanks.

fhahn accepted this revision.Aug 16 2023, 3:12 AM

LGTM, thanks for adding the extra test coverage. If possible, it would be good to also add a short description to each test on why it shouldn't be vectorized to help readers in the future/.

This revision is now accepted and ready to land.Aug 16 2023, 3:12 AM
This revision was landed with ongoing or failed builds.Aug 30 2023, 5:10 AM
This revision was automatically updated to reflect the committed changes.