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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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);
----------------
Ayal wrote:
> use dyn_cast instead of isa&cast?
Done, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4332
   setDebugLocFromInst(Builder, ReductionStartValue);
   bool IsInLoopReductionPhi = Cost->isInLoopReduction(OrigPhi);
 
----------------
Ayal wrote:
> bool IsInLoopReductionPhi = PhiR->isInLoop()?
I updated the code to just use `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();
----------------
Ayal wrote:
> (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(?)
> )
> isOrdered() implies isInLoop()

Now only `isOrdered` is check. The VPReductionPHIRecipe constructor ensures `IsInLoop` is set whenever `IsOrdered` is true.

> IsOrdered here should be IsOrderedAndVectorized

Removed the variable, use `isOrdered` instead.

> another place worth defining LastPartForVecRdxPhi

Done!

> wonder what happens when isOrdered()&NotVectorized wires UF parts(?)

After recent changes, there's no need to check for `isVector`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4348
+  bool IsOrdered =
+      State.VF.isVector() && IsInLoopReductionPhi && PhiR->isOrdered();
 
----------------
kmclaughlin wrote:
> Hi @fhahn, I recently created D104533 to fix ordered reductions where the VF = 1. After this change, the `State.VF.isVector()` check here (and in VPReductionPHIRecipe::execute) is no longer required.
Thanks, removed!


================
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);
----------------
Ayal wrote:
> Are both PrevBB and VectorPreHeader needed?
Not really, we can use LoopInfo to get the pre-header block. Updated the code.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1139
+  auto &Builder = State.Builder;
+  bool IsOrdered = State.VF.isVector() && this->IsOrdered;
+
----------------
Ayal wrote:
> IsOrdered applies to VF=1 as well, right? Rename variable to IsOrderedAndVectorized?
No need to check for `isVector`, as @kmclaughlin mentioned. I just replaced the variable with `isOrdered()` checks.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:308
     BasicBlock *LastBB = nullptr;
 
+    BasicBlock *VectorPreHeader = nullptr;
----------------
Ayal wrote:
> Comment?
The member is now gone again.


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

Sounds like a good idea. I think it would be best to split up the existing WidenPHIRecipe first, to avoid moving too much at the same time.

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

Done, added First/Last values to the enum.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1129
+
+  /// The phi is part of an ordered reduction.
+  bool IsOrdered;
----------------
Ayal wrote:
> Implies reduction is also in-loop.
Added that `IsOrdered` also requires `IsInLoop`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1162
 
-  RecurrenceDescriptor *getRecurrenceDescriptor() { return RdxDesc; }
+  bool isOrdered() const { return IsOrdered; }
 };
----------------
Ayal wrote:
> Add `bool isInLoop()`?
Done, also added doc-comments.


================
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;
----------------
Ayal wrote:
> && !isa<VPReductionPHIRecipe>(UI) ?
Yes, updated! But I don't think it is possible to create a test case that shows the issue, because it would require a non-widenable reduction instruction I think.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:104
     VPVWidenPHISC,
+    VPVReductionPHISC,
     VPVWidenSelectSC,
----------------
Ayal wrote:
> keep lexical order?
> or keep all Phi's together?
I moved them together, so the range check is more efficient. Also added `VPFirstPHISC``/VPLastPHISC` to facilitate the range check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104989