[PATCH] D143465: [LoopVectorize] Vectorize the reduction pattern of integer min/max with index.

[Mel Chen via Phabricator via llvm-commits]

Mel-Chen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:379
+
+  PHINode *UserRecurPhi = nullptr;
+
----------------
fhahn wrote:
> It would be helpful to document how the new system of recurrences depending on other recurrences would work I think, possibly also with an explanation of the whole approach in the patch description.
Sure. I will document the whole approach. and update in the summary tomorrow.
Quickly explain the function of `UserRecurPhi` . The purpose of `UserRecurPhi` is to allow the recurrence to be used in the loop (loop internal use), and to ensure that the user is also a recurrence. `UserRecurPhi` will record the candidate user recurrence phi, and `UserRecurKind` will recored the excepted user recurrence kind. Currently I'm limiting candidates to one, but it should be possible to have more than one.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:758
+  //   select(cmp(), loop_induction, phi)
+  if (isIncreasingLoopInduction(NonPhi))
+    return InstDesc(I, isa<ICmpInst>(I->getOperand(0))
----------------
fhahn wrote:
> The naming here is a bit confusing now, `NonPhi` can be an increasing loop induction? In that case it would be a phi, right?
Yes, it's a little confusing here. It could be better to replace `NonPhi` with `NonRecurPhi`. By the way, are you interested in supporting full functional SelectCmp pattern? I think min max with index pattern really needs to depend on the SelectCmp to be safe.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:777
+                 4>
+      DependRecurrenceMasks;
 };
----------------
fhahn wrote:
> We are in the process of removing those kinds of global maps that are used to carry information used during codegen and later. Ideally the combination of values would be modeled explicitly in the exit block of the plan, but we are not there yet. This is the main reason for D132063 doing things the way it does.
I see., but `DependRecurrenceMasks` exists for a reason. Consider the following case:

```
int idx = ii;
int foo = jj;
int max = mm;
for (int i = 0; i < n; ++i) {
  int x = a[i];
  if (max < x) {
    max = x;
    idx = i;
    foo = b[i];
  }
}
```
That mask has the chance to be reused, and I try to keep that flexibility. Of course, we can recalculate the mask for each reduction that needs a mask, but currently using the global maps to preserve the mask is a relatively simple method that I think of.
I have heard that VPlan is going to be extended to other blocks, could you share the relevant discussion links?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3875
+
+  while (!Worklist.empty()) {
+    VPRecipeBase &R = *(Worklist.front());
----------------
fhahn wrote:
> this would need documenting.
Sure. Quick explanation: min/max recurrence should be done earlier than min max idx recurrence, because idx recurrence depends on the mask produced by min max recurrence. Here is to ensure that the recurrence dependencies are correct.


================
Comment at: llvm/test/Transforms/LoopVectorize/smax-idx.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=loop-vectorize -force-vector-width=4 -force-vector-interleave=1 -debug-only=loop-vectorize,iv-descriptors -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK-VF4IC1 --check-prefix=CHECK
----------------
fhahn wrote:
> Could you add new tests as a separate patch?
Of course. I will split an NFC patch tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143465