[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: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.
 };
----------------
Ayal wrote:
> Should in general apply to any type, including pointer etc.
Indeed!


================
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:
> 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?
Of course, we can remove `isSelectCmpRecurrenceKind()` in this patch. After rebasing D150851, I'll temporarily provide a restored implementation of `isSelectCmpRecurrenceKind()`. Then, we can discuss whether to keep it, rename it, or directly use `isAnyOfRecurrenceKind() || isFindLastIVRecurrenceKind()`.


================
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:
> 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);
> ```
> ?
Sure, we can discuss later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155786