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

[Mel Chen via Phabricator via llvm-commits]

Mel-Chen marked an inline comment as done.
Mel-Chen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:44
+  SMax,     ///< Signed integer max implemented in terms of select(cmp()).
+  UMin,     ///< Unisgned integer min implemented in terms of select(cmp()).
+  UMax,     ///< Unsigned integer max implemented in terms of select(cmp()).
----------------
Mel-Chen wrote:
> Ayal wrote:
> > 
> LG, this change will push directly to llvm-project repo.
Fixed  https://github.com/llvm/llvm-project/commit/59629429028bfadccdf937436c0eb722bd556378


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:52
+  FMaximum, ///< FP max with llvm.maximum semantics
+  FMulAdd,  ///< Fused multiply-add of floats (a * b + c).
+  IAnyOf,   ///< Integer select(icmp(),x,y) where one of (x,y) is loop invariant
----------------
Mel-Chen wrote:
> Ayal wrote:
> > 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]).
> LG, this change will push directly to llvm-project repo.
Fixed https://github.com/llvm/llvm-project/commit/59629429028bfadccdf937436c0eb722bd556378


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:237
   ///   select(cmp(),x,y) where one of (x,y) is loop invariant.
   static bool isSelectCmpRecurrenceKind(RecurKind Kind) {
+    return Kind == RecurKind::IAnyOf || Kind == RecurKind::FAnyOf;
----------------
Mel-Chen wrote:
> Ayal wrote:
> > Mel-Chen wrote:
> > > Ayal wrote:
> > > > Would it make (more) sense to rename this `isAnyOfRecurrenceKind()`?
> > > I suggest this:
> > > ```
> > > static bool isAnyOfRecurrenceKind(RecurKind Kind) {
> > >   return Kind == RecurKind::IAnyOf || Kind == RecurKind::FAnyOf;
> > > }
> > > 
> > > static bool isSelectCmpRecurrenceKind(RecurKind Kind) {
> > >   return isAnyOfRecurrenceKind(Kind);
> > > }
> > > ```
> > Will `isSelectCmpRecurrenceKind()` still be needed?
> > Min/max may also uses a compare-select pattern, leading to potential confusion of "isSelectCmpRecurrenceKind()".
> 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.
> 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.




================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:806-807
   case Instruction::Call:
     if (isSelectCmpRecurrenceKind(Kind))
       return isSelectCmpPattern(L, OrigPhi, I, Prev);
     auto HasRequiredFMF = [&]() {
----------------
Ayal wrote:
> ?
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.


================
Comment at: llvm/test/Transforms/LoopVectorize/smax-idx.ll:91
 ;
-; Currently SelectICmp does not support icmp with multiple users.
+; Currently SelectCmp does not support icmp with multiple users.
 ; It may be possible to reuse some of the methods in Combination pass to check
----------------
Ayal wrote:
> Mel-Chen wrote:
> > Ayal wrote:
> > > Indeed not an any_of reduction, but an SMax one.
> > > Would be good to clarify this TODO comment (`Combination pass`?)
> > 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.
> > 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`.
Yes, it is instcombine pass. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155786/new/

https://reviews.llvm.org/D155786