[PATCH] D152693: LoopVectorize: introduce RecurKind::Induction(I|F)(Max|Min)
Ramkumar Ramachandra via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 7 04:24:54 PDT 2023
artagnon added a comment.
Herald added a subscriber: wangpc.
In D152693#4455117 <https://reviews.llvm.org/D152693#4455117>, @Mel-Chen wrote:
> 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.
I implemented this patch based on a TSVC run: gcc-aarch64 vectorizes increasing and decreasing induction variables, while llvm-riscv does not.
Thanks for your review comments: I will keep them in mind when developing a follow-up patch, when your patch lands.
================
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;
+ }
----------------
Mel-Chen wrote:
> Actually, I've had an idea: for this types of RecurKind, we should only need to do `AddReductionVar` once.
Yes, your approach of extending `isSelectCmpPattern` saves us these.
================
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, ...}.
----------------
Mel-Chen wrote:
> Based on your implementation, `Right` would not be what you stated as {0, 1, 331, 331, 331, ...}, but rather {331, 331, 331, 331, 331, ...}.
Ah yes, my error.
================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1112
+ Builder.CreateSelect(CmpVector, Src, MinSplat, "rdx.select.mask");
+ NewVal = Builder.CreateIntMaxReduce(Mask, Desc.isSigned());
+ break;
----------------
Mel-Chen wrote:
> 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.
> ```
>
Oh, ouch.
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