[PATCH] D104989: [VPlan] Add VPReductionPHIRecipe (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4135
     auto *OrigPhi = cast<PHINode>(PhiR->getUnderlyingValue());
-    if (PhiR->getRecurrenceDescriptor()) {
-      fixReduction(PhiR, State);
+    if (isa<VPReductionPHIRecipe>(PhiR->getDef())) {
+      fixReduction(cast<VPReductionPHIRecipe>(PhiR->getDef()), State);
----------------
use dyn_cast instead of isa&cast?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4332
   setDebugLocFromInst(Builder, ReductionStartValue);
   bool IsInLoopReductionPhi = Cost->isInLoopReduction(OrigPhi);
 
----------------
bool IsInLoopReductionPhi = PhiR->isInLoop()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4347
 
-  bool IsOrdered = State.VF.isVector() && IsInLoopReductionPhi &&
-                   Cost->useOrderedReductions(RdxDesc);
+  bool IsOrdered =
+      State.VF.isVector() && IsInLoopReductionPhi && PhiR->isOrdered();
----------------
(unrelated to this patch, but:
  - isOrdered() implies isInLoop()
  - `IsOrdered` here should be `IsOrderedAndVectorized`
  - another place worth defining LastPartForVecRdxPhi
  - wonder what happens when isOrdered()&NotVectorized wires UF parts(?)
)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4757
     bool ScalarPHI =
         (State.VF.isScalar()) || Cost->isInLoopReduction(cast<PHINode>(PN));
     Type *VecTy =
----------------
Drop Cost->isInLoopReduction, this now deals w/ FOR only

(wonder if FOR works well for VF=1, e.g., w/o sinkAfter(?))


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4766
     }
     if (Legal->isFirstOrderRecurrence(P))
       return;
----------------
Drop isFOR, we now know it is


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4771
   assert(!Legal->isReductionVariable(P) &&
          "reductions should be handled above");
 
----------------
"above" >> "elsewhere"


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8043
   State.CFG.PrevBB = ILV.createVectorizedLoopSkeleton();
+  State.CFG.VectorPreHeader = State.CFG.PrevBB;
   State.TripCount = ILV.getOrCreateTripCount(nullptr);
----------------
Are both PrevBB and VectorPreHeader needed?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1139
+  auto &Builder = State.Builder;
+  bool IsOrdered = State.VF.isVector() && this->IsOrdered;
+
----------------
IsOrdered applies to VF=1 as well, right? Rename variable to IsOrderedAndVectorized?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:308
     BasicBlock *LastBB = nullptr;
 
+    BasicBlock *VectorPreHeader = nullptr;
----------------
Comment?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:733
   /// Returns true for PHI-like recipes.
   bool isPhi() const {
+    return getVPDefID() == VPWidenIntOrFpInductionSC ||
----------------
Introduce a base class common to all phi recipes to ask isa<> instead of isPhi?

Check range of SC's (from FirstPHI to LastPHI) instead of multiple ||'s, or make sure they compile down to a range check?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1059
 
+protected:
 public:
----------------
VPWidenPHIRecipe(VPVID, VPDefID, Phi) constructor should be 'protected'? Else drop 'protected'.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1129
+
+  /// The phi is part of an ordered reduction.
+  bool IsOrdered;
----------------
Implies reduction is also in-loop.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1162
 
-  RecurrenceDescriptor *getRecurrenceDescriptor() { return RdxDesc; }
+  bool isOrdered() const { return IsOrdered; }
 };
----------------
Add `bool isInLoop()`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:279
           continue;
-        auto *PhiR = dyn_cast<VPWidenPHIRecipe>(UI);
-        if (PhiR && !PhiR->getRecurrenceDescriptor())
+        if (isa<VPWidenPHIRecipe>(UI))
           return true;
----------------
&& !isa<VPReductionPHIRecipe>(UI) ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:104
     VPVWidenPHISC,
+    VPVReductionPHISC,
     VPVWidenSelectSC,
----------------
keep lexical order?
or keep all Phi's together?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:331
     VPWidenPHISC,
+    VPReductionPHISC,
     VPWidenSC,
----------------
ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104989