[llvm] r302876 - [IfConversion] Keep the CFG updated incrementally in IfConvertTriangle

Tobias Grosser via llvm-commits llvm-commits at lists.llvm.org
Wed May 24 22:02:49 PDT 2017


Hi Michael,

I suspect this commit triggered a regression in the AOSP buildbot [1]
which we just saw now (as it was hidden by other errors):

You can see the error in one of its recent build logs:

http://lab.llvm.org:8011/builders/aosp-O3-polly-before-vectorizer-unprofitable/builds/132/steps/build-aosp/logs/stdio

where we see the following backtrace:

clang++:
/var/lib/buildbot/slaves/hexagon-build-03/aosp/llvm.src/lib/CodeGen/MachineBasicBlock.cpp:569:
MachineBasicBlock::succ_iterator
llvm::MachineBasicBlock::removeSuccessor(succ_iterator, bool): Assertion
`I != Successors.end() && "Not a current successor!"' failed.
#0 0x00000000014cf494 PrintStackTraceSignalHandler(void*)
(llvm.inst/bin/clang+++0x14cf494)
#1 0x00000000014cf7a6 SignalHandler(int)
(llvm.inst/bin/clang+++0x14cf7a6)
#2 0x00007f4c5c043d10 __restore_rt
(/lib/x86_64-linux-gnu/libpthread.so.0+0x10d10)
#3 0x00007f4c5ac21267 gsignal
/build/buildd/glibc-2.21/signal/../sysdeps/unix/sysv/linux/raise.c:55:0
#4 0x00007f4c5ac22eca abort /build/buildd/glibc-2.21/stdlib/abort.c:91:0
#5 0x00007f4c5ac1a03d __assert_fail_base
/build/buildd/glibc-2.21/assert/assert.c:92:0
#6 0x00007f4c5ac1a0f2 (/lib/x86_64-linux-gnu/libc.so.6+0x2e0f2)
#7 0x0000000000d7829c
llvm::MachineBasicBlock::removeSuccessor(__gnu_cxx::__normal_iterator<llvm::MachineBasicBlock**,
std::vector<llvm::MachineBasicBlock*,
std::allocator<llvm::MachineBasicBlock*> > >, bool)
(llvm.inst/bin/clang+++0xd7829c)
#8 0x0000000000d3f317 (anonymous
namespace)::IfConverter::runOnMachineFunction(llvm::MachineFunction&)
(llvm.inst/bin/clang+++0xd3f317)
#9 0x0000000000d9f009
llvm::MachineFunctionPass::runOnFunction(llvm::Function&)
(llvm.inst/bin/clang+++0xd9f009)
#10 0x000000000108b004
llvm::FPPassManager::runOnFunction(llvm::Function&)
(llvm.inst/bin/clang+++0x108b004)
#11 0x000000000108b253 llvm::FPPassManager::runOnModule(llvm::Module&)
(llvm.inst/bin/clang+++0x108b253)
#12 0x000000000108b745 llvm::legacy::PassManagerImpl::run(llvm::Module&)
(llvm.inst/bin/clang+++0x108b745)
#13 0x000000000165d3cf
clang::EmitBackendOutput(clang::DiagnosticsEngine&,
clang::HeaderSearchOptions const&, clang::CodeGenOptions const&,
clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout
const&, llvm::Module*, clang::BackendAction,
std::unique_ptr<llvm::raw_pwrite_stream,
std::default_delete<llvm::raw_pwrite_stream> >)
(llvm.inst/bin/clang+++0x165d3cf)
#14 0x0000000001e2b870
clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&)
(llvm.inst/bin/clang+++0x1e2b870)
#15 0x00000000020be055 clang::ParseAST(clang::Sema&, bool, bool)
(llvm.inst/bin/clang+++0x20be055)
#16 0x0000000001a1e3e8 clang::FrontendAction::Execute()
(llvm.inst/bin/clang+++0x1a1e3e8)
#17 0x00000000019e2511
clang::CompilerInstance::ExecuteAction(clang::FrontendAction&)
(llvm.inst/bin/clang+++0x19e2511)
#18 0x0000000001aa9045
clang::ExecuteCompilerInvocation(clang::CompilerInstance*)
(llvm.inst/bin/clang+++0x1aa9045)
#19 0x00000000007e4503 cc1_main(llvm::ArrayRef<char const*>, char
const*, void*) (llvm.inst/bin/clang+++0x7e4503)
#20 0x00000000007e2af2 main (llvm.inst/bin/clang+++0x7e2af2)
#21 0x00007f4c5ac0ca40 __libc_start_main
/build/buildd/glibc-2.21/csu/libc-start.c:323:0
#22 0x00000000007df7e9 _start (llvm.inst/bin/clang+++0x7df7e9)

Eli might be able to get you a reduced test case within one of the
following days, but maybe this already rings a bell?

Best,
Tobias

[1]
http://lab.llvm.org:8011/builders/aosp-O3-polly-before-vectorizer-unprofitable

On Fri, May 12, 2017, at 08:28 AM, Mikael Holmen via llvm-commits wrote:
> Author: uabelho
> Date: Fri May 12 01:28:58 2017
> New Revision: 302876
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=302876&view=rev
> Log:
> [IfConversion] Keep the CFG updated incrementally in IfConvertTriangle
> 
> Summary:
> Instead of using RemoveExtraEdges (which uses analyzeBranch, which cannot
> always be trusted) at the end to fixup the CFG we keep the CFG updated as
> we go along and remove or add branches and merge blocks.
> 
> This way we won't have any problems if the involved MBBs contain
> unanalyzable instructions.
> 
> This fixes PR32721.
> 
> In that case we had a triangle
> 
>    EBB
>    | \
>    |  |
>    | TBB
>    |  /
>    FBB
> 
> where FBB didn't have any successors at all since it ended with an
> unconditional return. Then TBB and FBB were be merged into EBB, but EBB
> would still keep its successors, and the use of analyzeBranch and
> CorrectExtraCFGEdges wouldn't help to remove them since the return
> instruction is not analyzable (at least not on ARM).
> 
> Reviewers: kparzysz, iteratee, MatzeB
> 
> Reviewed By: iteratee
> 
> Subscribers: aemerson, rengolin, javed.absar, llvm-commits
> 
> Differential Revision: https://reviews.llvm.org/D33037
> 
> Added:
>     llvm/trunk/test/CodeGen/MIR/ARM/PR32721_ifcvt_triangle_unanalyzable.mir
> Modified:
>     llvm/trunk/lib/CodeGen/IfConversion.cpp
> 
> Modified: llvm/trunk/lib/CodeGen/IfConversion.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/IfConversion.cpp?rev=302876&r1=302875&r2=302876&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/IfConversion.cpp (original)
> +++ llvm/trunk/lib/CodeGen/IfConversion.cpp Fri May 12 01:28:58 2017
> @@ -1588,22 +1588,32 @@ bool IfConverter::IfConvertTriangle(BBIn
>      BBCvt = MBPI->getEdgeProbability(BBI.BB, &CvtMBB);
>    }
>  
> +  // To be able to insert code freely at the end of BBI we sometimes
> remove
> +  // the branch from BBI to NextMBB temporarily. Remember if this
> happened.
> +  bool RemovedBranchToNextMBB = false;
>    if (CvtMBB.pred_size() > 1) {
>      BBI.NonPredSize -= TII->removeBranch(*BBI.BB);
>      // Copy instructions in the true block, predicate them, and add them
>      to
>      // the entry block.
>      CopyAndPredicateBlock(BBI, *CvtBBI, Cond, true);
>  
> -    // RemoveExtraEdges won't work if the block has an unanalyzable
> branch, so
> -    // explicitly remove CvtBBI as a successor.
> +    // Keep the CFG updated.
>      BBI.BB->removeSuccessor(&CvtMBB, true);
>    } else {
>      // Predicate the 'true' block after removing its branch.
>      CvtBBI->NonPredSize -= TII->removeBranch(CvtMBB);
>      PredicateBlock(*CvtBBI, CvtMBB.end(), Cond);
>  
> -    // Now merge the entry of the triangle with the true block.
> +    // Remove the branch from the entry of the triangle to NextBB to be
> able to
> +    // do the merge below. Keep the CFG updated, but remember we removed
> the
> +    // branch since we do want to execute NextMBB, either by introducing
> a
> +    // branch to it again, or merging it into the entry block.
> +    // How it's handled is decided further down.
>      BBI.NonPredSize -= TII->removeBranch(*BBI.BB);
> +    BBI.BB->removeSuccessor(&NextMBB, true);
> +    RemovedBranchToNextMBB = true;
> +
> +    // Now merge the entry of the triangle with the true block.
>      MergeBlocks(BBI, *CvtBBI, false);
>    }
>  
> @@ -1641,12 +1651,19 @@ bool IfConverter::IfConvertTriangle(BBIn
>      // block. By not merging them, we make it possible to iteratively
>      // ifcvt the blocks.
>      if (!HasEarlyExit &&
> -        NextMBB.pred_size() == 1 && !NextBBI->HasFallThrough &&
> +        // We might have removed BBI from NextMBB's predecessor list
> above but
> +        // we want it to be there, so consider that too.
> +        (NextMBB.pred_size() == (RemovedBranchToNextMBB ? 0 : 1)) &&
> +        !NextBBI->HasFallThrough &&
>          !NextMBB.hasAddressTaken()) {
> +      // We will merge NextBBI into BBI, and thus remove the current
> +      // fallthrough from BBI into CvtBBI.
> +      BBI.BB->removeSuccessor(&CvtMBB, true);
>        MergeBlocks(BBI, *NextBBI);
>        FalseBBDead = true;
>      } else {
>        InsertUncondBranch(*BBI.BB, NextMBB, TII);
> +      BBI.BB->addSuccessor(&NextMBB);
>        BBI.HasFallThrough = false;
>      }
>      // Mixed predicated and unpredicated code. This cannot be
>      iteratively
> @@ -1654,8 +1671,6 @@ bool IfConverter::IfConvertTriangle(BBIn
>      IterIfcvt = false;
>    }
>  
> -  RemoveExtraEdges(BBI);
> -
>    // Update block info. BB can be iteratively if-converted.
>    if (!IterIfcvt)
>      BBI.IsDone = true;
> 
> Added:
> llvm/trunk/test/CodeGen/MIR/ARM/PR32721_ifcvt_triangle_unanalyzable.mir
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/MIR/ARM/PR32721_ifcvt_triangle_unanalyzable.mir?rev=302876&view=auto
> ==============================================================================
> ---
> llvm/trunk/test/CodeGen/MIR/ARM/PR32721_ifcvt_triangle_unanalyzable.mir
> (added)
> +++
> llvm/trunk/test/CodeGen/MIR/ARM/PR32721_ifcvt_triangle_unanalyzable.mir
> Fri May 12 01:28:58 2017
> @@ -0,0 +1,24 @@
> +# RUN: llc -mtriple=arm-apple-ios -run-pass=if-converter %s -o - |
> FileCheck %s
> +---
> +name:            foo
> +body:             |
> +  bb.0:
> +    B %bb.2
> +
> +  bb.1:
> +    BX_RET 14, 0
> +
> +  bb.2:
> +    Bcc %bb.1, 1, %cpsr
> +
> +  bb.3:
> +    B %bb.1
> +
> +...
> +
> +# We should get a single block containing the BX_RET, with no successors
> at all
> +
> +# CHECK:      body:
> +# CHECK-NEXT:   bb.0:
> +# CHECK-NEXT:     BX_RET
> +
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


More information about the llvm-commits mailing list