[PATCH] D158333: [VPlan] Move initial skeleton construction to createInitialVPlan. (NFC)
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 19 04:56:15 PDT 2023
Ayal added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:723
+ auto *TopRegion = new VPRegionBlock("vector loop", false /*isReplicator*/);
+ VPBlockUtils::insertBlockAfter(TopRegion, Plan->getEntry());
+ VPBasicBlock *MiddleVPBB = new VPBasicBlock("middle.block");
----------------
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2544
+ /// which contains SCEV expansions that need to happen before the CFG is
+ /// modified; a VPbasicBlock for the vector pre-header, followed by a region
+ /// for the vector loop, followed by the middle VPBasicBlock.
----------------
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:307
+ // 0. Reuse the top-level region, vector-preheader and exit VPBBs from the
+ // skeleton.
+ VPRegionBlock *TheRegion = Plan.getVectorLoopRegion();
----------------
// These were created directly rather than via getOrCreateVPBB(), revisit them now to update BB2VPBB and Loop2region.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:313
+ "Unexpected loop preheader");
+ VPBasicBlock *VectorPreheaderVPBB = Plan.getEntry();
+ BB2VPBB[ThePreheaderBB] = VectorPreheaderVPBB;
----------------
nit:
` auto *VectorPreheaderVPBB = cast<VPBasicBlock>(TheRegion->getSinglePredecessor());`
would be more consistent with how middle VPBB is retrieved.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:314
+ VPBasicBlock *VectorPreheaderVPBB = Plan.getEntry();
+ BB2VPBB[ThePreheaderBB] = VectorPreheaderVPBB;
+ BasicBlock *LoopExitBB = TheLoop->getUniqueExitBlock();
----------------
Worth noting that `ThePreheaderBB` conceptually corresponds to both Plan.getPreheader() (which wraps the original preheader BB) and Plan.getEntry() (which represents the new vector preheader); here we're interested in setting BB2VPBB to the latter.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:328
// LoopBlocksDFS.
- BasicBlock *ThePreheaderBB = TheLoop->getLoopPreheader();
- assert((ThePreheaderBB->getTerminator()->getNumSuccessors() == 1) &&
- "Unexpected loop preheader");
- VPBasicBlock *ThePreheaderVPBB = Plan.getEntry();
- BB2VPBB[ThePreheaderBB] = ThePreheaderVPBB;
- ThePreheaderVPBB->setName("vector.ph");
+ VectorPreheaderVPBB->setName("vector.ph");
for (auto &I : *ThePreheaderBB) {
----------------
Is this redundant - createInitialVPlan() already sets the name of VectorPreheaderVPBB to "vector.ph" when creating it?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:337
+ HeaderVPBB->setName("vector.body");
+ HeaderVPBB->setParent(TheRegion);
+ TheRegion->setEntry(HeaderVPBB);
----------------
Is setting the parent of HeaderVPBB to TheRegion redundant - getOrCreateVPBB() should have done so already?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:338
+ HeaderVPBB->setParent(TheRegion);
+ TheRegion->setEntry(HeaderVPBB);
----------------
Should this setEntry be done along with all others?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:354
assert(LI->getLoopFor(BB)->getHeader() == BB);
- setRegionPredsFromBB(Region, BB);
+ if (TheRegion != Region)
+ setRegionPredsFromBB(Region, BB);
----------------
nit: may be simpler to handle the non-header case first, then potentially else-if the header-of-non-TheRegion case.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:384
Successors[0] == Region ? Successors[1] : Successors[0];
Region->setExiting(VPBB);
Region->setParent(ExitVPBB->getParent());
----------------
If Exits of regions are set here when identifying latches during successor setting, should Entries be set above when identifying headers during predecessor setting?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:386
Region->setParent(ExitVPBB->getParent());
- Successors = {ExitVPBB};
+ if (TheRegion != Region)
+ Successors = {ExitVPBB};
----------------
nit: continue if TheRegion == Region?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158333/new/
https://reviews.llvm.org/D158333
More information about the llvm-commits
mailing list