[PATCH] D152693: LoopVectorize: introduce RecurKind::Induction(I|F)(Max|Min)

[Mel Chen via Phabricator via llvm-commits]

Mel-Chen added a comment.

I'm glad that others have also noticed this, especially the implementation of the select-cmp pattern for decreasing induction variables. The select-cmp pattern for decreasing induction variables is a function that I haven't implemented yet. Have you come across any real applications or benchmarks that make use of it.



================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:55-62
+  InductionIMax, ///< Integer select(icmp(), x, y) where one of (x, y) is
+                 ///< the induction variable to be maximized.
+  InductionIMin, ///< Integer select(icmp(), x, y) where one of (x, y) is
+                 ///< the induction variable to be minimized.
+  InductionFMax, ///< Integer select(fcmp(), x, y) where one of (x, y) is
+                 ///< the induction variable to be maximized.
+  InductionFMin  ///< Integer select(fcmp(), x, y) where one of (x, y) is
----------------
I can understand the literal meaning of "the induction variable to be maximized/minimized," but perhaps there is a better way to describe this pattern, such as "monotonic increasing" and "monotonic decreasing."


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:741
+                                           : RecurKind::InductionFMax);
+      return InstDesc(I, ConstStep->getAPInt().isNegative() ? RMin : RMax);
+    }
----------------
 I recommend using isKnownNegative.
```
bool 	isKnownNegative (const SCEV *S)
 	Test if the given expression is known to be negative.
```


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1021-1026
+  if (AddReductionVar(Phi, RecurKind::InductionFMin, TheLoop, FMF, RedDes, DB,
+                      AC, DT, SE)) {
+    LLVM_DEBUG(dbgs() << "Found a MIN reduction on loop induction variable PHI."
+                      << *Phi << "\n");
+    return true;
+  }
----------------
Actually, I've had an idea: for this types of RecurKind, we should only need to do `AddReductionVar` once.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1174-1177
+  case RecurKind::InductionIMax:
+  case RecurKind::InductionIMin:
+  case RecurKind::InductionFMax:
+  case RecurKind::InductionFMin:
----------------
Based on my understanding, in a broader sense, ternary operations do not have an identity. Although I used the term "identity" for convenience in implementation to refer to the sentinel value, I have been considering whether to separate the concept of the sentinel value from identity.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1094
+  // 2. The Src is filled with values: {0, 1, 2, 3, 4, ...}.
+  // 3. The Right is filled with values: {0, 1, 331, 331, 331, ...}.
+  // 4. The CmpVector is filled with values: {1, 1, 0, 0, 0, ...}.
----------------
Based on your implementation, `Right` would not be what you stated as {0, 1, 331, 331, 331, ...}, but rather {331, 331, 331, 331, 331, ...}.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1112
+        Builder.CreateSelect(CmpVector, Src, MinSplat, "rdx.select.mask");
+    NewVal = Builder.CreateIntMaxReduce(Mask, Desc.isSigned());
+    break;
----------------
The behavior of `isSigned` is different from what you have in mind. (Indeed, the name is really misleading.)

```
bool 	isSigned () const
 	Returns true if all source operands of the recurrence are SExtInsts.
```



================
Comment at: llvm/test/Transforms/LoopVectorize/select-min-index.ll:2-3
 ; RUN: opt -passes=loop-vectorize -force-vector-width=4 -force-vector-interleave=1 -S %s | FileCheck %s
-; RUN: opt -passes=loop-vectorize -force-vector-width=4 -force-vector-interleave=2 -S %s | FileCheck %s
-; RUN: opt -passes=loop-vectorize -force-vector-width=1 -force-vector-interleave=2 -S %s | FileCheck %s
 
----------------
Why skip interleave?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152693