[PATCH] D123005: [VPlan] Use region for each loop in native path.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8672
   VPBasicBlock *EB = TopRegion->getExitBasicBlock();
   if (IsVPlanNative) {
     EB->setCondBit(nullptr);
----------------
Ayal wrote:
> nit: can also remove brackets
Should be fixed!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:269
     // and latch blocks. As a result, we never enter this function for the
     // header block in the non VPlan-native path.
     if (!PredBB) {
----------------
Ayal wrote:
> Comment still intact?
This is not relevant any longer, removed!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:284
       assert(PredVPSuccessors.size() == 1 &&
              "Predecessor ending w/o branch must have single successor.");
+      if (TermBr) {
----------------
Ayal wrote:
> update error message?
> Simpler to retain the "if (isa<UnreachableInst>(...)) {" and add an "else if (auto *TermBr = dyn_cast<BranchInst>(...))"?

Unfortunately the diff was showing changes that were landed as part of a different review. I applied the suggestions in e66127e69bfa. That applies for all suggestions for changes in VPlan.cpp.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:293
+      }
     } else {
+      if (PredVPSuccessors.size() == 2) {
----------------
Ayal wrote:
> } else if {
Simplified, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:299
+        PredBBTerminator->setSuccessor(idx, NewBB);
+      } else {
+        auto *Reg = dyn_cast<VPRegionBlock>(PredVPBB->getParent());
----------------
Ayal wrote:
> worth a comment?
> In this case PredVPBB is the exit block of a loop region - PredVPSuccessors.size() is zero?
Added a comment, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:302
+        assert(Reg && !Reg->isReplicator());
+        assert(this == Reg->getSingleSuccessor());
+        PredBBTerminator->setSuccessor(0, NewBB);
----------------
Ayal wrote:
> error messages? cast instead of dyn_cast?
> assert PredVPSuccessors is empty and PredBBTerminator has no successors?
> assert PredVPBB == Reg->getExitBasicBlock()?
> Reg>>LoopRegion, as Reg may also refer to Register(?)
Renamed and added extra assertions, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:313
 void VPBasicBlock::execute(VPTransformState *State) {
+  if (!getParent() && getNumSuccessors() == 0)
+    return;
----------------
Ayal wrote:
> Worth a comment - why have a(n exit) block with recipes if it's execute is skipped?
> This is not instead of the continues in RPOT traversal below, right?
After re-ordering the patches, this is not needed any longer (originally this patch was placed before introducing any recipes in the exit)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:321
 
+  auto GetEnclosingNonReplicateRegion = [](VPBasicBlock *VPBB) {
+    VPRegionBlock *P = VPBB->getParent();
----------------
Ayal wrote:
> "NonReplicateRegion" >> "LoopRegion"?
This has been simplified in the latest version, now there's just a single `IsLoopRegion` helper.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:345
+        PrevVPBB->getSingleHierarchicalSuccessor() &&
+        (SingleHPred->getParent() == GetEnclosingNonReplicateRegion(this) &&
+         !IsNonReplicateR(SingleHPred))) &&      /* B */
----------------
Ayal wrote:
> Augment explanation of case B above. The case of SingleHPred being a LoopRegion is clear, what is the other case of SingleHPred residing in same LoopRegion enclosing this VPBB?
> 
> Need the extra brackets around
> 
> ```
>   SingleHPred->getParent() == GetEnclosingNonReplicateRegion(this) &&
>   !IsNonReplicateR(SingleHPred)?
> ```
> ?
Should be updated in the latest version


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:946
   State->CFG.ExitBB = VectorHeaderBB->getSingleSuccessor();
+  State->CFG.PrevBB = VectorHeaderBB;
   State->CurrentVectorLoop = State->LI->getLoopFor(VectorHeaderBB);
----------------
Ayal wrote:
> nit: why is setting CFG.PrevBB moved?
Not moved in the latest version.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:948
   State->CurrentVectorLoop = State->LI->getLoopFor(VectorHeaderBB);
 
+  if (isa<VPBasicBlock>(getEntry())) {
----------------
Ayal wrote:
> Explanation why if entry is a VPBB (preheader?) then VectorHeaderBB is merged into its predecessor - VectorPreHeader?
This is also gone in the latest version.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:40
 #include "llvm/Analysis/LoopInfo.h"
+#include "llvm/Analysis/VectorUtils.h"
 #include "llvm/IR/DebugLoc.h"
----------------
Ayal wrote:
> nit: formatting fix independent of this patch
Should be fixed, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2645
+    if (R)
+      return R;
+    return cast<VPRegionBlock>(getEntry()->getSingleSuccessor());
----------------
Ayal wrote:
> May as well return to current version(?) doing
> 
> ```
>      if (auto *R = dyn_cast<VPRegionBlock>(getEntry()))
>        return R;
> ```
This now just does ` cast<VPRegionBlock>(getEntry()->getSingleSuccessor())`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:77
 
-  // Build the plain CFG and return its Top Region.
-  VPRegionBlock *buildPlainCFG();
+  /// Build plain CFG for TheLoop. Return the pre-header VPBasicBlock connected
+  /// to a new VPRegionBlock (TopRegion) enclosing the plain CFG.
----------------
Ayal wrote:
> typo: TheLoop
I *think* this should be fixed.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:86
+// backedge of the loop is modeled implicitly through the containing region
+// block.
+void PlainCFGBuilder::setVPBBPredsFromBB(VPBasicBlock *VPBB, BasicBlock *BB,
----------------
Ayal wrote:
> \p HeaderBB should be \p LatchBB in order to skip the backedge when it is a predecessor?
HeaderBB/LatchBB is gone in the latest version.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:287
+  PreheaderVPBB->setOneSuccessor(HeaderVPBB);
+
   LoopBlocksRPO RPO(TheLoop);
----------------
Ayal wrote:
> Why move?
> Comment no longer true - Preheader's predecessors no longer set during RPO traversal?
> Why move?

The move was accidental, but the comment is not longer true because the predecessor doesn't have any predecessors.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:323
 
+      VPValue *VPCondBit = IRDef2VPValue[BrCond];
       // Link successors using condition bit.
----------------
Ayal wrote:
> Why move?
Undone.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:341
+  //  Loop exit was already set as successor of the loop exiting BB.
+  //  We only set its predecessor VPBB now.
+  setVPBBPredsFromBB(LoopExitVPBB, LoopExitBB, nullptr);
----------------
Ayal wrote:
> Why added spaces?
Should be removed.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:360
+    VPBlockUtils::disconnectBlocks(HeaderPred, HeaderVPBB);
+    VPBlockUtils::disconnectBlocks(LatchVPBB, HeaderVPBB);
+    Region->setParent(HeaderPred->getParent());
----------------
Ayal wrote:
> Backedge was not connected above?
In the latest version, the backedge is connect earlier and removed here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123005