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

[Mel Chen via Phabricator via llvm-commits]

Mel-Chen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:55-57
+  SelectMinIdx, ///< Select of the index with minimal value, needs to be
+                ///< combined with SelectMinIdxMinVal
+  SelectMinIdxMinVal,
----------------
Interesting. I have different point. In my opinion, we should be able to reuse min/max recurrence. This prevents adding Select[Min|Max]Idx for each kind of min/max. I will share my current implementation later.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:130
                                     RecurKind Kind, InstDesc &Prev,
-                                    FastMathFlags FuncFMF);
+                                    FastMathFlags FuncFMF, ScalarEvolution *SE);
 
----------------
ABataev wrote:
> Pass by reference?
I use pointer too. This is because function `isReductionPHI` may not pass argument SE.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:912-918
+  InductionDescriptor ID;
+  if (!isa<PHINode>(U) ||
+      !InductionDescriptor::isInductionPHI(cast<PHINode>(U), TheLoop, &SE, ID))
+    return nullptr;
+
+  if (!ID.getStep()->isOne() || ID.getInductionOpcode() != Instruction::Add)
+    return nullptr;
----------------
Canonical induction variable? It seems that there is a existing function that can be called directly, but I forgot the function name...


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1224-1225
   case RecurKind::UMin:
+  case RecurKind::SelectMinIdxMinVal:
     return ConstantInt::get(Tp, -1);
+  case RecurKind::SelectMinIdx:
----------------
I also encountered the same problem here. I think MMI should base on full SelectCmp support. Using -1 may cause problems with unsigned index. But full support for SelectCmp is another long long story.


================
Comment at: llvm/test/Transforms/LoopVectorize/select-min-index.ll:26-28
 exit:
   %res = phi i64 [ %min.idx.next, %loop ]
   ret i64 %res
----------------
Your test case is single-exit. This is TODO for me. I will add a case that returns both umin and index.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132063