[llvm] 59f49f7 - [IR] Simplify BasicBlock::removePredecessor. NFCI.
Richard Smith via llvm-commits
llvm-commits at lists.llvm.org
Tue May 19 16:18:11 PDT 2020
Hi, I'm seeing two SimplifyCFG tests crashing after this commit. Backtrace
is:
<segfault handler invoked>
#5 0x000056442fcb15d4 llvm::PHINode::getBasicBlockIndex(llvm::BasicBlock
const*) const
#6 0x00005644324808b3 (anonymous
namespace)::SimplifyCFGOpt::simplifyCommonResume(llvm::ResumeInst*)
#7 0x000056443246e65c (anonymous
namespace)::SimplifyCFGOpt::simplifyResume(llvm::ResumeInst*,
llvm::IRBuilder<llvm::ConstantFolder, llvm::IRBuilderDefaultInserter>&)
#8 0x000056443246cc03 (anonymous
namespace)::SimplifyCFGOpt::simplifyOnce(llvm::BasicBlock*)
#9 0x000056443246c780 (anonymous
namespace)::SimplifyCFGOpt::run(llvm::BasicBlock*)
Failing tests are:
test/Transforms/SimplifyCFG/bug-25299.ll
test/Transforms/SimplifyCFG/invoke_unwind.ll
On Tue, 19 May 2020 at 11:42, Jay Foad via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
> Author: Jay Foad
> Date: 2020-05-19T19:34:49+01:00
> New Revision: 59f49f7ee7f3397e000f7e11facb4a5605cd1cab
>
> URL:
> https://github.com/llvm/llvm-project/commit/59f49f7ee7f3397e000f7e11facb4a5605cd1cab
> DIFF:
> https://github.com/llvm/llvm-project/commit/59f49f7ee7f3397e000f7e11facb4a5605cd1cab.diff
>
> LOG: [IR] Simplify BasicBlock::removePredecessor. NFCI.
>
> Differential Revision: https://reviews.llvm.org/D80141
>
> Added:
>
>
> Modified:
> llvm/include/llvm/IR/BasicBlock.h
> llvm/lib/IR/BasicBlock.cpp
>
> Removed:
>
>
>
>
> ################################################################################
> diff --git a/llvm/include/llvm/IR/BasicBlock.h
> b/llvm/include/llvm/IR/BasicBlock.h
> index 1210ed1eb2ef..312d2a0569ad 100644
> --- a/llvm/include/llvm/IR/BasicBlock.h
> +++ b/llvm/include/llvm/IR/BasicBlock.h
> @@ -370,12 +370,8 @@ class BasicBlock final : public Value, // Basic
> blocks are data objects also
> /// except operator delete.
> void dropAllReferences();
>
> - /// Notify the BasicBlock that the predecessor \p Pred is no longer
> able to
> - /// reach it.
> - ///
> - /// This is actually not used to update the Predecessor list, but is
> actually
> - /// used to update the PHI nodes that reside in the block. Note that
> this
> - /// should be called while the predecessor still refers to this block.
> + /// Update PHI nodes in this BasicBlock before removal of predecessor
> \p Pred.
> + /// Note that this function does not actually remove the predecessor.
> void removePredecessor(BasicBlock *Pred, bool KeepOneInputPHIs = false);
>
> bool canSplitPredecessors() const;
>
> diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
> index a9fa7cae92d8..fb4639c675d5 100644
> --- a/llvm/lib/IR/BasicBlock.cpp
> +++ b/llvm/lib/IR/BasicBlock.cpp
> @@ -317,58 +317,31 @@ iterator_range<BasicBlock::phi_iterator>
> BasicBlock::phis() {
> return make_range<phi_iterator>(P, nullptr);
> }
>
> -/// This method is used to notify a BasicBlock that the
> -/// specified Predecessor of the block is no longer able to reach it.
> This is
> -/// actually not used to update the Predecessor list, but is actually
> used to
> -/// update the PHI nodes that reside in the block. Note that this should
> be
> -/// called while the predecessor still refers to this block.
> -///
> +/// Update PHI nodes in this BasicBlock before removal of predecessor \p
> Pred.
> +/// Note that this function does not actually remove the predecessor.
> void BasicBlock::removePredecessor(BasicBlock *Pred,
> bool KeepOneInputPHIs) {
> - assert((hasNUsesOrMore(16)||// Reduce cost of this assertion for
> complex CFGs.
> + // Use hasNUsesOrMore to bound the cost of this assertion for complex
> CFGs.
> + assert((hasNUsesOrMore(16) ||
> find(pred_begin(this), pred_end(this), Pred) != pred_end(this))
> &&
> - "removePredecessor: BB is not a predecessor!");
> -
> - if (InstList.empty()) return;
> - PHINode *APN = dyn_cast<PHINode>(&front());
> - if (!APN) return; // Quick exit.
> -
> - // If there are exactly two predecessors, then we want to nuke the PHI
> nodes
> - // altogether.
> - unsigned max_idx = APN->getNumIncomingValues();
> - assert(max_idx != 0 && "PHI Node in block with 0 predecessors!?!?!");
> -
> - // <= Two predecessors BEFORE I remove one?
> - if (max_idx <= 2 && !KeepOneInputPHIs) {
> - // Yup, loop through and nuke the PHI nodes
> - while (PHINode *PN = dyn_cast<PHINode>(&front())) {
> - // Remove the predecessor first.
> - PN->removeIncomingValue(Pred, !KeepOneInputPHIs);
> -
> - // If the PHI _HAD_ two uses, replace PHI node with its now
> *single* value
> - if (max_idx == 2) {
> - if (PN->getIncomingValue(0) != PN)
> - PN->replaceAllUsesWith(PN->getIncomingValue(0));
> - else
> - // We are left with an infinite loop with no entries: kill the
> PHI.
> - PN->replaceAllUsesWith(UndefValue::get(PN->getType()));
> - getInstList().pop_front(); // Remove the PHI node
> - }
> + "Pred is not a predecessor!");
>
> - // If the PHI node already only had one entry, it got deleted by
> - // removeIncomingValue.
> - }
> - } else {
> - // Okay, now we know that we need to remove predecessor #pred_idx
> from all
> - // PHI nodes. Iterate over each PHI node fixing them up
> - PHINode *PN;
> - for (iterator II = begin(); (PN = dyn_cast<PHINode>(II)); ) {
> - ++II;
> - PN->removeIncomingValue(Pred, false);
> - // If all incoming values to the Phi are the same, we can replace
> the Phi
> - // with that value.
> - Value* PNV = nullptr;
> - if (!KeepOneInputPHIs && (PNV = PN->hasConstantValue())) {
> + // Return early if there are no PHI nodes to update.
> + if (!isa<PHINode>(begin()))
> + return;
> + unsigned NumPreds = cast<PHINode>(front()).getNumIncomingValues();
> +
> + // Update all PHI nodes.
> + for (iterator II = begin(); isa<PHINode>(II);) {
> + PHINode *PN = cast<PHINode>(II++);
> + PN->removeIncomingValue(Pred);
> + // If we have a single predecessor, removeIncomingValue erased the
> PHI node
> + // itself.
> + // FIXME in practice "KeepOneInputPHIs" means "KeepConstantPHIs" and
> some
> + // callers seem to rely on that.
> + if (NumPreds > 1 && !KeepOneInputPHIs) {
> + if (Value *PNV = PN->hasConstantValue()) {
> + // Replace the PHI node with its constant value.
> PN->replaceAllUsesWith(PNV);
> PN->eraseFromParent();
> }
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200519/90cbb560/attachment.html>
More information about the llvm-commits
mailing list