[PATCH] D132063: [LV] Support vectorizing 'select index of minimum element' idiom. (WIP)

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:130
                                     RecurKind Kind, InstDesc &Prev,
-                                    FastMathFlags FuncFMF);
+                                    FastMathFlags FuncFMF, ScalarEvolution *SE);
 
----------------
Pass by reference?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:835
+/// otherwise.
+static Instruction *isMinIdxReductionMinVal(PHINode *Phi, Loop *TheLoop,
+                                            ScalarEvolution &SE) {
----------------
Better to name it something like get..., since it returns instruction?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:843-845
+    if (Phi->getIncomingValueForBlock(TheLoop->getLoopLatch()) != UMinU)
+      return false;
+    return true;
----------------
Just `return Phi->getIncomingValueForBlock(TheLoop->getLoopLatch()) == UMinU;`


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:861
+
+static Instruction *isMinIdxReductionMinIdx(PHINode *IdxPhi, Loop *TheLoop,
+                                            ScalarEvolution &SE) {
----------------
get?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:883-894
+  if (auto *P = dyn_cast<PHINode>(CmpOp0)) {
+    if ((UMin = isMinIdxReductionMinVal(P, TheLoop, SE))) {
+      MinPhi = P;
+      CurVal = CmpOp1;
+    }
+  } else if (auto *P = dyn_cast<PHINode>(CmpOp1)) {
+    if ((UMin = isMinIdxReductionMinVal(P, TheLoop, SE))) {
----------------
```
auto *P0 = dyn_cast<PHINode>(CmpOp0);
auto *P1 = dyn_cast<PHINode>(CmpOp1);
auto *P = P0 ? P0 : P1;
if (!P)
  return nullptr;
if ((UMin = isMinIdxReductionMinVal(P, TheLoop, SE))) {
   MinPhi = P;
   CurVal = P == P0 ? CmpOp1 : CmpOp0;
}
```


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1029
+  }
+  if (Instruction *ExitI = isMinIdxReductionMinVal(Phi, TheLoop, *SE)) {
+    FastMathFlags FMF;
----------------
The condition is same as in previous if. Is this ok?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3965
   Value *ReducedPartRdx = State.get(LoopExitInstDef, 0);
+
   unsigned Op = RecurrenceDescriptor::getOpcode(RK);
----------------
Remove the new line


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4000-4001
+    VPReductionPHIRecipe *MinP = nullptr;
+    for (VPRecipeBase &P : PhiR->getParent()
+                               ->getPlan()
+                               ->getVectorLoopRegion()
----------------
State.Plan?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4011-4012
+        MinP = RedP;
+        RD2 = &RdxDesc;
+      }
+    }
----------------
break?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8678
              Phi->getIncomingValueForBlock(OrigLoop->getLoopPreheader()));
+
       PhiRecipe = new VPReductionPHIRecipe(Phi, RdxDesc, *StartV,
----------------
Delete new line


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:645-647
+        if (!B->getType()->isVectorTy()) {
+          B = Builder.CreateVectorSplat(State.VF, B);
+        }
----------------
No need for braces


================
Comment at: llvm/test/Transforms/LoopVectorize/select-min-index.ll:8
 ; CHECK-LABEL: @test_vectorize_select_umin_idx(
-; CHECK-NOT:   vector.body:
+; CHECK: vector.body:
 ;
----------------
Generate the checks by the script?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132063