[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 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()).
----------------
Ayal wrote:
> 
LG, this change will push directly to llvm-project repo.


================
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
----------------
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.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:53-54
+  FMulAdd,  ///< Fused multiply-add of floats (a * b + c).
+  IAnyOf,   ///< Integer select(icmp(),x,y) where one of (x,y) is loop invariant
+  FAnyOf    ///< Integer select(fcmp(),x,y) where one of (x,y) is loop invariant
 };
----------------
Ayal wrote:
> 
Since currently we only have implemented the integer version of `any_of`, would it be better to retain it specifically for integers?


================
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;
----------------
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);
}
```


================
Comment at: llvm/test/Transforms/LoopVectorize/smax-idx.ll:58
 
 ; Check if it is a MMI when smax is not used outside the loop.
 ;
----------------
Ayal wrote:
> nit: `MMI` - Min-or-Max-with-Index?
> 
> (nit: if index is not used outside the loop, best eliminate its computation as dead code.)
> 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.


================
Comment at: llvm/test/Transforms/LoopVectorize/smax-idx.ll:62
 ; But in fact MMI should be possible to use the exitInstruction of
-; SelectICmp be the exitInstruction.
+; SelectCmp be the exitInstruction.
 ;
----------------
Ayal wrote:
> Indeed not an any_of reduction, but an SMax one.
> Would be good to clarify this TODO comment, also grammatically (`exitInstruction`? `SelectCmp`?).
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.
```


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155786