[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 a comment.

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



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



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


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



================
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;
----------------
Would it make (more) sense to rename this `isAnyOfRecurrenceKind()`?


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:402
+/// described by \p Desc.
 Value *createSelectCmpTargetReduction(IRBuilderBase &B,
                                       const TargetTransformInfo *TTI,
----------------
`createAnyOfTargetReduction()`?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:3267
+  case RecurKind::IAnyOf:
+  case RecurKind::FAnyOf:
   case RecurKind::FMulAdd:
----------------
nit: while we're here, worth placing these last - after `FMulAdd`, as they appear in RecurKind definition?


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h:292
+    case RecurKind::IAnyOf:
+    case RecurKind::FAnyOf:
     case RecurKind::FMulAdd:
----------------
ditto


================
Comment at: llvm/test/Transforms/LoopVectorize/smax-idx.ll:58
 
 ; Check if it is a MMI when smax is not used outside the loop.
 ;
----------------
nit: `MMI` - Min-or-Max-with-Index?

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


================
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.
 ;
----------------
Indeed not an any_of reduction, but an SMax one.
Would be good to clarify this TODO comment, also grammatically (`exitInstruction`? `SelectCmp`?).


================
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
----------------
Indeed not an any_of reduction, but an SMax one.
Would be good to clarify this TODO comment (`Combination pass`?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155786