[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-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
 };
----------------
Mel-Chen wrote:
> Ayal wrote:
> > 
> Since currently we only have implemented the integer version of `any_of`, would it be better to retain it specifically for integers?
> 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.


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


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


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1124
   RecurKind RK = Desc.getRecurrenceKind();
   if (RecurrenceDescriptor::isSelectCmpRecurrenceKind(RK))
+    return createAnyOfTargetReduction(B, TTI, Src, Desc, OrigPhi);
----------------
?


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


================
Comment at: llvm/test/Transforms/LoopVectorize/smax-idx.ll:58
 
 ; Check if it is a MMI when smax is not used outside the loop.
 ;
----------------
Mel-Chen wrote:
> 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.
Ah, right, thanks for clarifying.



================
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.
 ;
----------------
Mel-Chen wrote:
> 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.
> ```
> 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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155786