[llvm] 59f49f7 - [IR] Simplify BasicBlock::removePredecessor. NFCI.

Wed May 20 00:08:25 PDT 2020

Sorry. I'll revert and then investigate. Those tests passed in my debug build.

Jay.

On Wed, 20 May 2020 at 00:19, Richard Smith via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits