[PATCH] D113182: [VPlan] Wrap vector loop blocks in region.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9309
 
   // Create a dummy pre-entry VPBasicBlock to start building the VPlan.
   auto Plan = std::make_unique<VPlan>();
----------------
Ayal wrote:
> Is this comment about dummy pre-entry obsolete?
Yes! It should have been dropped as part of a6c4969f5f45. Removed in 76effb001d33.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9328
+      TopRegion->setEntry(FirstVPBBForBB);
+      Plan->setEntry(TopRegion);
+    }
----------------
Ayal wrote:
> nit: can alternatively fold into a 1-liner by feeding FirstVPBBForBB to the constructor of VPRegionBlock, but above is more informative.
I kept it as is for now, preferring the more informative version. But I'm happy to adjust it if you prefer that.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9397
          !Plan->getEntry()->getEntryBasicBlock()->empty() &&
          "entry block must be set to a non-empty VPBasicBlock");
+  cast<VPRegionBlock>(Plan->getEntry())->setExit(VPBB);
----------------
Ayal wrote:
> "entry block must be set to a VPRegionBlock having a non-empty entry VPBasicBlock");
Updated, thanks!


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll:11
 ; CHECK:        VPlan 'Initial VPlan for VF={vscale x 2},UF>=1' {
+; CHECK-NEXT:   <x1> vector loop: {
 ; CHECK-NEXT:   loop.body:
----------------
Ayal wrote:
> (<x1> indicating that this region is not replicating; i.e., is a loop region.)
Yes, unfortunately this is not very explicit. Should we change that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113182