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

[Ramkumar Ramachandra via Phabricator via llvm-commits]

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