This is an archive of the discontinued LLVM Phabricator instance.

[LV] Add tests for select-cmp reduction pattern. (NFC)
ClosedPublic

Authored by Mel-Chen on Jun 27 2023, 8:36 PM.

Details

Summary

The test cases for selecting increasing integer induction variable.

Diff Detail

Event Timeline

Mel-Chen created this revision.Jun 27 2023, 8:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2023, 8:36 PM
Herald added a subscriber: artagnon. · View Herald Transcript
Mel-Chen requested review of this revision.Jun 27 2023, 8:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2023, 8:36 PM
fhahn added inline comments.Jul 10 2023, 12:47 PM
llvm/test/Transforms/LoopVectorize/iv-select-cmp-no-wrap.ll
12

Should we also have tests with different induction start values (constant and variable)? Would be good to adjust the name of the phis to make clearer what's the induction and what's the select reduction.

nit: personally I find it easier to read if the incoming value from the preheader comes first.

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

What does _const_2 stand for here?

Mel-Chen added inline comments.Jul 10 2023, 10:53 PM
llvm/test/Transforms/LoopVectorize/iv-select-cmp-no-wrap.ll
12

Sure. I will add a new test case into iv-select-cmp.ll, and it will be a negative case for D150851.

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

There is no special meaning. _const is only used to indicate that the value is compared to a constant, and _2 is just a serial number indicating they are similar tests. The difference between const_2 and const_1 is %cond = select i1 %cmp2, i64 %idx.09, i64 %i.010, while the difference between const_3 and const_1 is %idx.09 = phi i64 [ %cond, %for.body ], [ %ii, %entry ].

Mel-Chen updated this revision to Diff 538965.Jul 11 2023, 1:56 AM

Changes:

  • Change the name of phi
  • Add a negative test case
shiva0217 accepted this revision.Jul 12 2023, 11:11 PM

LGTM

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

Perhaps select_icmp_min_valid_iv_start ?

This revision is now accepted and ready to land.Jul 12 2023, 11:11 PM
Mel-Chen updated this revision to Diff 541854.Jul 18 2023, 11:54 PM

Changes:

  • Replace select_icmp_min_iv_start_value with select_icmp_min_valid_iv_start
fhahn accepted this revision.Jul 19 2023, 2:47 AM

LGTM, thanks!

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

nit: can also drop .010 and .09 from the value names

50

nit: would select_icmp_const_3-> select_icmp_const_3_variable_rdx_start be more descriptive?

116

nit: ii better named rdx.start or something like that

nit: would @select_icmp -> @select_icmp_variable_rdx_start be more descriptive?

Mel-Chen updated this revision to Diff 542285.Jul 19 2023, 8:15 PM
Mel-Chen marked an inline comment as done.

Changes:

  • Drop the suffix of variable name
  • Replace select_icmp_const_3 with select_icmp_const_3_variable_rdx_start
  • Replace %ii with %rdx.start
This revision was landed with ongoing or failed builds.Jul 19 2023, 8:17 PM
This revision was automatically updated to reflect the committed changes.