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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


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


================
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) {
----------------
Comment still intact?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:285
              "Predecessor ending w/o branch must have single successor.");
-      DebugLoc DL = PredBBTerminator->getDebugLoc();
-      PredBBTerminator->eraseFromParent();
-      auto *Br = BranchInst::Create(NewBB, PredBB);
-      Br->setDebugLoc(DL);
+      if (TermBr) {
+        TermBr->setSuccessor(0, NewBB);
----------------
remove brackets


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:293
+      }
     } else {
+      if (PredVPSuccessors.size() == 2) {
----------------
} else if {


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:295
+      if (PredVPSuccessors.size() == 2) {
+        unsigned idx = PredVPSuccessors.front() == this ? 0 : 1;
+        assert(!PredBBTerminator->getSuccessor(idx) &&
----------------
(unrelated to this patch): assert PredVPSuccessors[idx] == this?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:302
+        assert(Reg && !Reg->isReplicator());
+        assert(this == Reg->getSingleSuccessor());
+        PredBBTerminator->setSuccessor(0, NewBB);
----------------
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(?)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:313
 void VPBasicBlock::execute(VPTransformState *State) {
+  if (!getParent() && getNumSuccessors() == 0)
+    return;
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:321
 
+  auto GetEnclosingNonReplicateRegion = [](VPBasicBlock *VPBB) {
+    VPRegionBlock *P = VPBB->getParent();
----------------
"NonReplicateRegion" >> "LoopRegion"?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:328
+
+  auto IsNonReplicateR = [](VPBlockBase *BB) {
+    auto *R = dyn_cast<VPRegionBlock>(BB);
----------------
ditto


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:345
+        PrevVPBB->getSingleHierarchicalSuccessor() &&
+        (SingleHPred->getParent() == GetEnclosingNonReplicateRegion(this) &&
+         !IsNonReplicateR(SingleHPred))) &&      /* B */
----------------
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)?
```
?


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:40
 #include "llvm/Analysis/LoopInfo.h"
+#include "llvm/Analysis/VectorUtils.h"
 #include "llvm/IR/DebugLoc.h"
----------------
nit: formatting fix independent of this patch


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2645
+    if (R)
+      return R;
+    return cast<VPRegionBlock>(getEntry()->getSingleSuccessor());
----------------
May as well return to current version(?) doing

```
     if (auto *R = dyn_cast<VPRegionBlock>(getEntry()))
       return R;
```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2651
+    if (R)
+      return R;
+    return cast<VPRegionBlock>(getEntry()->getSingleSuccessor());
----------------
ditto


================
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.
----------------
typo: TheLoop


================
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,
----------------
\p HeaderBB should be \p LatchBB in order to skip the backedge when it is a predecessor?


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


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


================
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);
----------------
Why added spaces?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:360
+    VPBlockUtils::disconnectBlocks(HeaderPred, HeaderVPBB);
+    VPBlockUtils::disconnectBlocks(LatchVPBB, HeaderVPBB);
+    Region->setParent(HeaderPred->getParent());
----------------
Backedge was not connected above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123005