[PATCH] D152693: LoopVectorize: introduce RecurKind::Induction(I|F)(Max|Min)
Mel Chen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 28 01:10:05 PDT 2023
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
More information about the llvm-commits
mailing list