[llvm] r302876 - [IfConversion] Keep the CFG updated incrementally in IfConvertTriangle
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Sun May 28 22:12:55 PDT 2017
Hi Tobias,
On 05/25/2017 07:02 AM, Tobias Grosser wrote:
> 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?
>
(Sorry for the late reply, we had thursday/friday off here in Sweden.)
Yes it rings a bell.
I'm in the process of rewriting the other parts of IfConversion as well
to not need to rely on RemoveExtraEdges/analyzeBranch, and during that
work I realized my original fix to IfConvertTriangle had a bug that
could lead to the above.
I intend to fix my introduced bug as well in my rewrite commit, but the
whole thing is taking a lot more time than I first anticipated, so I'm
totally fine if you want to revert r302876 in the meantime.
If you can manage to get an ll-reproducer I'd be happy so I can see that
it's not something else than the problem I'm aware of.
Thanks and sorry for the inconvenience,
Mikael
> 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