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

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 added inline comments.
Herald added a subscriber: wangpc.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1038
+    RecurrenceDescriptor &UserRedDes = Reductions.find(UserPhi)->second;
+    if (!RedDes.fixUserRecurrence(UserRedDes))
+      return false;
----------------
Instead of fixUserRecurrence to setDependMinMaxRecurDes and change the user RecurKind,  is it possible to setDependMinMaxRecurDes when isReductionPHI return true?

If we able to propagate parent(dependent) RecurDes to isReductionPHI, perhaps we can create reduction as following.

  RecurKind ParentKind = RedDes.getRecurrenceKind();
  if (ParentKind == RecurKind::SMax) {
    if (AddReductionVar(Phi, RecurKind::MinMaxFirstIdx, TheLoop, FMF, RedDes, DB, AC, DT,
                        SE)) {
      LLVM_DEBUG(dbgs() << "Found an MinMaxFirstIdx reduction PHI." << *Phi << "\n");
      return true;
    }
  }

The dependency for the RecurKind could be explicitly and avoid the user RecurKind fixup.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4033
+      else {
+        if (RdxDesc.hasUserRecurrence()) {
+          ReducedPartRdx =
----------------
Should we check isMinMaxRecurrenceKind(Kind) and isMinMaxIdxRecurrenceKind for user kind?

Although it would be the only dependency currently, it might be explicit for the reader and avoid unexpected codegen in the future.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4035
+          ReducedPartRdx =
+              createMinMaxSelectCmpOp(Builder, RK, ReducedPartRdx, RdxPart);
+          // Keep the part mask on demand.
----------------
Could we encapsulate the mask generation to createMinMaxIdxMaskOp or other name you prefer?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4060
+    // Create depend recurrence mask on demand.
+    if (RdxDesc.hasUserRecurrence()) {
+      ElementCount EC =
----------------
Should we check isMinMaxRecurrenceKind(Kind) and isMinMaxIdxRecurrenceKind for user kind?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4061
+    if (RdxDesc.hasUserRecurrence()) {
+      ElementCount EC =
+          cast<VectorType>(ReducedPart->getType())->getElementCount();
----------------
Could we encapsulate the mask generation to createMinMaxIdxMask or similar?




================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3875
+
+  while (!Worklist.empty()) {
+    VPRecipeBase &R = *(Worklist.front());
----------------
Mel-Chen wrote:
> 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.
Perhaps we could do the sorting according to the reduction dependency before calling fixReduction which may be similar to https://reviews.llvm.org/D157631.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143465