-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[NFC][LoopVectorize] Make replaceVPBBWithIRVPBB more efficient #111514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
389ab09
6c45224
3da4e4e
d1b45da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1004,13 +1004,9 @@ static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) { | |
assert(!R.isPhi() && "Tried to move phi recipe to end of block"); | ||
R.moveBefore(*IRVPBB, IRVPBB->end()); | ||
} | ||
VPBlockBase *PredVPBB = VPBB->getSinglePredecessor(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assert of having a single predecessor is now lost, yet still documented. The function traverses multiple predecessors, but probably works and tested for single predecessor only, as VPBB is still expected to be free of phi recipes? Better be clear, assert and test what is supported. |
||
VPBlockUtils::disconnectBlocks(PredVPBB, VPBB); | ||
VPBlockUtils::connectBlocks(PredVPBB, IRVPBB); | ||
for (auto *Succ : to_vector(VPBB->getSuccessors())) { | ||
VPBlockUtils::connectBlocks(IRVPBB, Succ); | ||
VPBlockUtils::disconnectBlocks(VPBB, Succ); | ||
} | ||
|
||
VPBlockUtils::reassociateBlocks(VPBB, IRVPBB); | ||
|
||
delete VPBB; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -440,6 +440,26 @@ class VPBlockBase { | |||||
Successors.erase(Pos); | ||||||
} | ||||||
|
||||||
/// This function replaces one predecessor with another, useful when | ||||||
/// trying to replace an old block in the CFG with a new one. | ||||||
void replacePredecessor(VPBlockBase *Old, VPBlockBase *New) { | ||||||
auto I = find(Predecessors, Old); | ||||||
assert(I != Predecessors.end()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error message missing. |
||||||
assert(Old->getParent() == New->getParent() && | ||||||
"replaced predecessor must have the same parent"); | ||||||
*I = New; | ||||||
} | ||||||
|
||||||
/// This function replaces one successor with another, useful when | ||||||
/// trying to replace an old block in the CFG with a new one. | ||||||
void replaceSuccessor(VPBlockBase *Old, VPBlockBase *New) { | ||||||
auto I = find(Successors, Old); | ||||||
assert(I != Successors.end()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error message missing. |
||||||
assert(Old->getParent() == New->getParent() && | ||||||
"replaced successor must have the same parent"); | ||||||
*I = New; | ||||||
} | ||||||
|
||||||
protected: | ||||||
VPBlockBase(const unsigned char SC, const std::string &N) | ||||||
: SubclassID(SC), Name(N) {} | ||||||
|
@@ -3862,6 +3882,19 @@ class VPBlockUtils { | |||||
To->removePredecessor(From); | ||||||
} | ||||||
|
||||||
/// Reassociate all the blocks connected to \p Old so that they now point to | ||||||
/// \p New. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
static void reassociateBlocks(VPBlockBase *Old, VPBlockBase *New) { | ||||||
for (auto *Pred : to_vector(Old->getPredecessors())) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this to_vector() needed, given that Old's predecessors remain intact? |
||||||
Pred->replaceSuccessor(Old, New); | ||||||
for (auto *Succ : to_vector(Old->getSuccessors())) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this to_vector() needed, given that Old's successors remain intact? |
||||||
Succ->replacePredecessor(Old, New); | ||||||
New->setPredecessors(Old->getPredecessors()); | ||||||
New->setSuccessors(Old->getSuccessors()); | ||||||
Old->clearPredecessors(); | ||||||
Old->clearSuccessors(); | ||||||
} | ||||||
|
||||||
/// Return an iterator range over \p Range which only includes \p BlockTy | ||||||
/// blocks. The accesses are casted to \p BlockTy. | ||||||
template <typename BlockTy, typename T> | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're here, worth adding in documentation above that VPBB, having a single predecessor, is expected to be free of phi recipes. A more informative error message may be "VPBB to be replaced by IRBB must be free of phi recipes".
Note that, in general, replacing a VPBB with an IRBB is a temporary solution, until the full skeleton is built in VPlan. Blocks would clearly be identified as IRBB's or not, from the outset.