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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:379
+
+  PHINode *UserRecurPhi = nullptr;
+
----------------
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.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:733
 
+  auto isIncreasingLoopInduction = [&SE, &Loop](Value *V) {
+    if (!SE)
----------------
nit: Variables should start with upper case 

also, move definition to use?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:758
+  //   select(cmp(), loop_induction, phi)
+  if (isIncreasingLoopInduction(NonPhi))
+    return InstDesc(I, isa<ICmpInst>(I->getOperand(0))
----------------
The naming here is a bit confusing now, `NonPhi` can be an increasing loop induction? In that case it would be a phi, right?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:777
+                 4>
+      DependRecurrenceMasks;
 };
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3875
+
+  while (!Worklist.empty()) {
+    VPRecipeBase &R = *(Worklist.front());
----------------
this would need documenting.


================
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
----------------
Could you add new tests as a separate patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143465