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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:53
+  FMulAdd,  ///< Sum of float products with llvm.fmuladd(a * b + sum).
+  IAnyOf, ///< Any_of reduction with select(icmp(),x,y) where one of (x,y) is an
+          ///< loop invariant, and both x and y are integer type.
----------------



================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:55
+          ///< loop invariant, and both x and y are integer type.
+  FAnyOf  ///< Any_of reduction with select(fcmp(),x,y) where one of (x,y) is an
+          ///< loop invariant, and both x and y are integer type.
----------------



================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:57-58
+          ///< loop invariant, and both x and y are integer type.
+  // TODO: Any_of reduction can be applied not only to integer types, but also
+  // to float types. In other words, x and y can also be of float type.
 };
----------------
Should in general apply to any type, including pointer etc.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1273
+      RecurrenceDescriptor::isAnyOfRecurrenceKind(RK)) {
     // MinMax reduction have the start value as their identify.
     if (ScalarPHI) {
----------------
(While we're here.)


================
Comment at: llvm/test/Transforms/LoopVectorize/smax-idx.ll:91
 
-; Check smax implemented in terms of select(cmp()).
+; Check smax implemented in format of select(cmp()).
 ;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155786