This is an archive of the discontinued LLVM Phabricator instance.

[LV] Rename the Select[I|F]Cmp reduction pattern to [I|F]AnyOf. (NFC)
ClosedPublic

Authored by Mel-Chen on Jul 19 2023, 11:45 PM.

Details

Summary

Regarding this NFC change, please refer to the discussion in this thread. https://reviews.llvm.org/D150851#4467261

Diff Detail

Event Timeline

Mel-Chen created this revision.Jul 19 2023, 11:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 11:45 PM
Mel-Chen requested review of this revision.Jul 19 2023, 11:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 11:45 PM
Mel-Chen edited the summary of this revision. (Show Details)Jul 19 2023, 11:56 PM

LGTM, but I'd wait for @Ayal's comments.

Ayal added a comment.Jul 21 2023, 12:46 PM

Thanks for following-up on this! Adding various minors nits.

llvm/include/llvm/Analysis/IVDescriptors.h
44
52

nit: while we're here, worth clarifying that FMulAdd is essentially an FAdd reduction whose reduced values are products fused with the add in fmuladd, as in a dot-product sum(a[i]*b[i]).

53–54
239–240

Would it make (more) sense to rename this isAnyOfRecurrenceKind()?

llvm/include/llvm/Transforms/Utils/LoopUtils.h
399–400

createAnyOfTargetReduction()?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
3287

nit: while we're here, worth placing these last - after FMulAdd, as they appear in RecurKind definition?

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
291

ditto

llvm/test/Transforms/LoopVectorize/smax-idx.ll
58–59

nit: MMI - Min-or-Max-with-Index?

(nit: if index is not used outside the loop, best eliminate its computation as dead code.)

61–64

Indeed not an any_of reduction, but an SMax one.
Would be good to clarify this TODO comment, also grammatically (exitInstruction? SelectCmp?).

92–93

Indeed not an any_of reduction, but an SMax one.
Would be good to clarify this TODO comment (Combination pass?)

Mel-Chen added inline comments.Jul 25 2023, 2:09 AM
llvm/include/llvm/Analysis/IVDescriptors.h
44

LG, this change will push directly to llvm-project repo.

52

LG, this change will push directly to llvm-project repo.

53–54

Since currently we only have implemented the integer version of any_of, would it be better to retain it specifically for integers?

239–240

I suggest this:

static bool isAnyOfRecurrenceKind(RecurKind Kind) {
  return Kind == RecurKind::IAnyOf || Kind == RecurKind::FAnyOf;
}

static bool isSelectCmpRecurrenceKind(RecurKind Kind) {
  return isAnyOfRecurrenceKind(Kind);
}
llvm/test/Transforms/LoopVectorize/smax-idx.ll
58–59

nit: MMI - Min-or-Max-with-Index?

Yes, it's min/max with index. I think I should put the full name before the short name.

(nit: if index is not used outside the loop, best eliminate its computation as dead code.)

This case checks if the maximum result is not used outside the loop. Regarding the index, if I recall correctly, the vectorizer should be able to perform dead code elimination for values that are not used outside the loop.

61–64

How do you feel about the following version?

; Check if it is a min/max with index (MMI) pattern when the
; min/max value is not used outside the loop.
;
; Currently, the vectorizer checks if smax value is used outside
; the loop. However, even if only the index part has external users,
; and smax itself does not have external users, it can still form a 
; MMI pattern.
92–93

Currently, all the reductions of the form select(cmp()) do not support cmp with more than one user, including min/max reductions.
However, FP min/max with index encounters the situation where cmp has more than one user.
Since the transformation of integer select(cmp(x, y), x, y) to [s|u]min/max appears to be done in the Combination pass, my plan is to refer to how multiple users of cmp are handled in that pass to relax the limitation for select(cmp()) reductions.

Mel-Chen updated this revision to Diff 543894.Jul 25 2023, 2:32 AM

Update the patch according to the comments.

Ayal added inline comments.Jul 26 2023, 2:32 AM
llvm/include/llvm/Analysis/IVDescriptors.h
53–54

Since currently we only have implemented the integer version of any_of, would it be better to retain it specifically for integers?

Ah, good clarification, worth a TODO somewhere, possibly attached to a negative test: this pattern is easily applicable to x,y of any type. How about:

IAnyOf,   ///< any_of reduction with select(icmp(),t,f) and invariant t or f (both integers)
IAnyOf    ///< any_of reduction with select(fcmp(),t,f) and invariant t or f (both integers)

AnyOf is essentially a boolean OR reduction, so should apply to both integer and floating point return values. The predicate producing the reduced values can be provided via icmp or fcmp. The end boolean result could be converted to a {true-value, false value} pair of any type, preferably after the loop. The types of true-value and false-value must match each other, but are independent of the type of comparison.

239–240

Will isSelectCmpRecurrenceKind() still be needed?
Min/max may also uses a compare-select pattern, leading to potential confusion of "isSelectCmpRecurrenceKind()".

llvm/lib/Analysis/IVDescriptors.cpp
804–806

?

llvm/lib/Transforms/Utils/LoopUtils.cpp
1123–1124

?

llvm/test/Transforms/LoopVectorize/smax-idx.ll
58–59

Ah, right, thanks for clarifying.

61–64

How do you feel about the following version?

LGTM.

Just noting that in some cases it suffices to report the index alone, e.g., when the smax value can be easily retrieved/recomputed using its index.

92–93

However, FP min/max with index encounters the situation where cmp has more than one user.

If min/max is computed with a cmp/select, this cmp will be reused to select its index.
If min is computed with an llvm.[smin|umin|minnum|minimum] intrinsic, and likewise for max, its index can be selected with its own cmp.

By Combination pass you mean instcombine? Anyhow, worth prefixing with TODO.

92–93

(All reductions require all operations along their chain to have a single user, except for a single live-out. But this test focuses on MMI, which would need to handle cmp feeding two selects as part of its chain.)

Mel-Chen marked 3 inline comments as done.Jul 30 2023, 11:36 PM
Mel-Chen added inline comments.
llvm/include/llvm/Analysis/IVDescriptors.h
239–240

Because eventually, D150851 will need isSelectCmpRecurrenceKind(). Both AnyOf and FindLastIV share common characteristics, such as similar identification methods. Using isSelectCmpRecurrenceKind() to control similar behavior should be better than isAnyOfRecurrenceKind() || isFindLastIVRecurrenceKind(). However, I also agree that isSelectCmpRecurrenceKind() might lead to confusion since the format of min max reduction is really similar.

Mel-Chen updated this revision to Diff 545542.Jul 31 2023, 1:57 AM
Mel-Chen marked an inline comment as done.

Rebase and update according to comments.

Mel-Chen marked an inline comment as done.Jul 31 2023, 2:03 AM
Mel-Chen added inline comments.
llvm/include/llvm/Analysis/IVDescriptors.h
44
52
239–240

Because eventually, D150851 will need isSelectCmpRecurrenceKind(). Both AnyOf and FindLastIV share common characteristics, such as similar identification methods. Using isSelectCmpRecurrenceKind() to control similar behavior should be better than isAnyOfRecurrenceKind() || isFindLastIVRecurrenceKind(). However, I also agree that isSelectCmpRecurrenceKind() might lead to confusion since the format of min max reduction is really similar.

llvm/lib/Analysis/IVDescriptors.cpp
804–806

I have not renamed isSelectCmpPattern() to isAnyOfPattern() yet. The reasons for this decision are explained in previous comment https://reviews.llvm.org/D155786/new/#inline-1515374.

llvm/test/Transforms/LoopVectorize/smax-idx.ll
92–93

Yes, it is instcombine pass.

Mel-Chen marked 8 inline comments as done and an inline comment as not done.Jul 31 2023, 2:06 AM
Mel-Chen added inline comments.
llvm/include/llvm/Analysis/IVDescriptors.h
53–54

Re-write comments and added TODO.

artagnon added inline comments.Jul 31 2023, 4:11 AM
llvm/test/Transforms/LoopVectorize/smax-idx.ll
95

Typo: duplicated.

Mel-Chen marked 2 inline comments as done.Jul 31 2023, 8:13 PM
Mel-Chen added inline comments.
llvm/test/Transforms/LoopVectorize/smax-idx.ll
95

Nice catch.

Mel-Chen updated this revision to Diff 545906.Jul 31 2023, 8:13 PM

Fixed typo.

Ayal added inline comments.Aug 1 2023, 3:54 PM
llvm/include/llvm/Analysis/IVDescriptors.h
53
55
57–58

Should in general apply to any type, including pointer etc.

239–240

Then let D150851 argue in favor of isSelectCmpRecurrenceKind() when it needs it, or be content with isAnyOfRecurrenceKind() || isFindLastIVRecurrenceKind() (which may be fine if used seldom). Here this appears redundant and useless?

llvm/lib/Analysis/IVDescriptors.cpp
804–806

Can this patch simply rename the existing code to read

if (isAnyOfRecurrenceKind(Kind))
  return isAnyOfPattern(L, OrigPhi, I, Prev);

leaving future support of FindLastIV in D150851 to add what it needs, e.g., possibly adding

if (isFindLastIVRecurrenceKind(Kind))
  return isFindLastIVPattern(L, OrigPhi, I, Prev);

?

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
1272–1273

(While we're here.)

llvm/test/Transforms/LoopVectorize/smax-idx.ll
91
Mel-Chen marked an inline comment as done.Aug 2 2023, 3:23 AM
Mel-Chen added inline comments.
llvm/include/llvm/Analysis/IVDescriptors.h
57–58

Indeed!

239–240

Of course, we can remove isSelectCmpRecurrenceKind() in this patch. After rebasing D150851, I'll temporarily provide a restored implementation of isSelectCmpRecurrenceKind(). Then, we can discuss whether to keep it, rename it, or directly use isAnyOfRecurrenceKind() || isFindLastIVRecurrenceKind().

llvm/lib/Analysis/IVDescriptors.cpp
804–806

Sure, we can discuss later.

Mel-Chen updated this revision to Diff 546382.Aug 2 2023, 3:24 AM

Change:

  • Replaced all SelectCmp.* with AnyOf.*
  • Fixed comments.
Ayal accepted this revision.Aug 2 2023, 7:08 AM

This looks good to me, thanks for the cleanup!
Adding a couple of minor nits.

llvm/include/llvm/Transforms/Utils/LoopUtils.h
368–371

Fixing this original documentation, while we're updating it: there is no \p LoopExitInst here.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4029–4032
This revision is now accepted and ready to land.Aug 2 2023, 7:08 AM
Mel-Chen marked 11 inline comments as done.Aug 3 2023, 12:30 AM
Mel-Chen added inline comments.
Mel-Chen updated this revision to Diff 546729.Aug 3 2023, 12:30 AM

Rebase, and update the comment.

This revision was landed with ongoing or failed builds.Aug 3 2023, 12:37 AM
This revision was automatically updated to reflect the committed changes.
Mel-Chen marked 2 inline comments as done.