[llvm] r311467 - [ADCE][Dominators] Reapply: Teach ADCE to preserve dominators

Jakub (Kuba) Kuderski via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 23 10:38:07 PDT 2017


Hi Adrian,

Expensive checks make .verifyDominatorTree to run O(n^3) sibling property
verification, which can cause some major slowndowns on big CFG's. I believe
that the question is how expensive we want the expensive checks to be -- if
that' unacceptable, then we can introduce a new flag just for that.

Please let me know what you think.

Best,
Kuba

On Aug 23, 2017 19:27, "Adrian Prantl" <aprantl at apple.com> wrote:

> A lot of jobs on green dragon are currently timing out, for example:
>
> http://green.lab.llvm.org/green/job/clang-stage1-cmake-
> RA-expensive/buildTimeTrend
>
> I logged into one of the build nodes and found several instances of clang,
> all running for >15min and attached LLDB to it:
>
> They were all stuck in llvm::DominatorTree::verifyDomTree().
>
> (lldb) bt
> * thread #1: tid = 0x1607b1d, 0x0000000108ea1776 clang-6.0`llvm::
> DomTreeNodeBase<llvm::BasicBlock>::compare(llvm::DomTreeNodeBase<llvm::BasicBlock>
> const*) const + 118, queue = 'com.apple.main-thread', stop reason = signal
> SIGSTOP
>   * frame #0: 0x0000000108ea1776 clang-6.0`llvm::DomTreeNodeBase<llvm::
> BasicBlock>::compare(llvm::DomTreeNodeBase<llvm::BasicBlock> const*)
> const + 118
>     frame #1: 0x0000000108ea1fb7 clang-6.0`llvm::DominatorTreeBase<llvm::BasicBlock,
> false>::compare(llvm::DominatorTreeBase<llvm::BasicBlock, false> const&)
> const + 327
>     frame #2: 0x0000000108eac121 clang-6.0`llvm::DominatorTree::verifyDomTree()
> const + 225
>     frame #3: 0x0000000108f060b9 clang-6.0`llvm::PMDataManager:
> :verifyPreservedAnalysis(llvm::Pass*) + 313
>     frame #4: 0x00000001089e6ccb clang-6.0`llvm::LPPassManager:
> :runOnFunction(llvm::Function&) + 1515
>     frame #5: 0x0000000108f09ea3 clang-6.0`llvm::FPPassManager:
> :runOnFunction(llvm::Function&) + 547
>     frame #6: 0x0000000108f0a103 clang-6.0`llvm::FPPassManager::runOnModule(llvm::Module&)
> + 51
>     frame #7: 0x0000000108f0a64e clang-6.0`llvm::legacy::
> PassManagerImpl::run(llvm::Module&) + 958
>     frame #8: 0x000000010971c264 clang-6.0`clang::
> EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions
> const&, clang::CodeGenOptions const&, clang::TargetOptions const&,
> clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*,
> clang::BackendAction, std::__1::unique_ptr<llvm::raw_pwrite_stream,
> std::__1::default_delete<llvm::raw_pwrite_stream> >) + 15444
>     frame #9: 0x0000000109974633 clang-6.0`clang::BackendConsumer::
> HandleTranslationUnit(clang::ASTContext&) + 947
>     frame #10: 0x000000010a3c2ab5 clang-6.0`clang::ParseAST(clang::Sema&,
> bool, bool) + 469
>     frame #11: 0x0000000109bfcfec clang-6.0`clang::FrontendAction::Execute()
> + 76
>     frame #12: 0x0000000109bb6d91 clang-6.0`clang::CompilerInstance::
> ExecuteAction(clang::FrontendAction&) + 1217
>     frame #13: 0x0000000109c6401a clang-6.0`clang::
> ExecuteCompilerInvocation(clang::CompilerInstance*) + 4970
>     frame #14: 0x0000000107c1c732 clang-6.0`cc1_main(llvm::ArrayRef<char
> const*>, char const*, void*) + 1394
>     frame #15: 0x0000000107c1ade3 clang-6.0`main + 11939
>     frame #16: 0x00007fff8fa725ad libdyld.dylib`start + 1
>
> Since you contributed a lot of changes to this code recently, do you think
> that this could be related to your commits? You should be able to reproduce
> this by building with expensive checks enabled and watching out for the
> builds of the ASAN testcases. ("Generating ASAN_.*_TEST_.*" in the build
> log)
>
> thanks,
> Adrian
>
> > On Aug 22, 2017, at 9:30 AM, Jakub Kuderski via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> > Author: kuhar
> > Date: Tue Aug 22 09:30:21 2017
> > New Revision: 311467
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=311467&view=rev
> > Log:
> > [ADCE][Dominators] Reapply: Teach ADCE to preserve dominators
> >
> > Summary:
> > This patch teaches ADCE to preserve both DominatorTrees and
> PostDominatorTrees.
> >
> > This is reapplies the original patch r311057 that was reverted in
> r311381.
> > The previous version wasn't using the batch update api for updating
> dominators,
> > which in vary rare cases caused assertion failures.
> >
> > This also fixes PR34258.
> >
> > Reviewers: dberlin, chandlerc, sanjoy, davide, grosser, brzycki
> >
> > Reviewed By: davide
> >
> > Subscribers: grandinj, zhendongsu, llvm-commits, david2050
> >
> > Differential Revision: https://reviews.llvm.org/D35869
> >
> > Added:
> >    llvm/trunk/test/Transforms/ADCE/2017-08-21-DomTree-deletions.ll
> >    llvm/trunk/test/Transforms/ADCE/domtree-DoubleDeletion.ll
> >    llvm/trunk/test/Transforms/ADCE/unreachable.ll
> > Modified:
> >    llvm/trunk/lib/Transforms/Scalar/ADCE.cpp
> >
> > Modified: llvm/trunk/lib/Transforms/Scalar/ADCE.cpp
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/Scalar/ADCE.cpp?rev=311467&r1=311466&r2=311467&view=diff
> > ============================================================
> ==================
> > --- llvm/trunk/lib/Transforms/Scalar/ADCE.cpp (original)
> > +++ llvm/trunk/lib/Transforms/Scalar/ADCE.cpp Tue Aug 22 09:30:21 2017
> > @@ -27,6 +27,7 @@
> > #include "llvm/IR/BasicBlock.h"
> > #include "llvm/IR/CFG.h"
> > #include "llvm/IR/DebugInfoMetadata.h"
> > +#include "llvm/IR/Dominators.h"
> > #include "llvm/IR/IRBuilder.h"
> > #include "llvm/IR/InstIterator.h"
> > #include "llvm/IR/Instructions.h"
> > @@ -89,6 +90,10 @@ struct BlockInfoType {
> >
> > class AggressiveDeadCodeElimination {
> >   Function &F;
> > +
> > +  // ADCE does not use DominatorTree per se, but it updates it to
> preserve the
> > +  // analysis.
> > +  DominatorTree &DT;
> >   PostDominatorTree &PDT;
> >
> >   /// Mapping of blocks to associated information, an element in
> BlockInfoVec.
> > @@ -157,9 +162,10 @@ class AggressiveDeadCodeElimination {
> >   void makeUnconditional(BasicBlock *BB, BasicBlock *Target);
> >
> > public:
> > -  AggressiveDeadCodeElimination(Function &F, PostDominatorTree &PDT)
> > -      : F(F), PDT(PDT) {}
> > -  bool performDeadCodeElimination();
> > + AggressiveDeadCodeElimination(Function &F, DominatorTree &DT,
> > +                               PostDominatorTree &PDT)
> > +     : F(F), DT(DT), PDT(PDT) {}
> > + bool performDeadCodeElimination();
> > };
> > }
> >
> > @@ -557,14 +563,34 @@ void AggressiveDeadCodeElimination::upda
> >     }
> >     assert((PreferredSucc && PreferredSucc->PostOrder > 0) &&
> >            "Failed to find safe successor for dead branch");
> > +
> > +    // Collect removed successors to update the (Post)DominatorTrees.
> > +    SmallPtrSet<BasicBlock *, 4> RemovedSuccessors;
> >     bool First = true;
> >     for (auto *Succ : successors(BB)) {
> > -      if (!First || Succ != PreferredSucc->BB)
> > +      if (!First || Succ != PreferredSucc->BB) {
> >         Succ->removePredecessor(BB);
> > -      else
> > +        RemovedSuccessors.insert(Succ);
> > +      } else
> >         First = false;
> >     }
> >     makeUnconditional(BB, PreferredSucc->BB);
> > +
> > +    // Inform the dominators about the deleted CFG edges.
> > +    SmallVector<DominatorTree::UpdateType, 4> DeletedEdges;
> > +    for (auto *Succ : RemovedSuccessors) {
> > +      // It might have happened that the same successor appeared
> multiple times
> > +      // and the CFG edge wasn't really removed.
> > +      if (Succ != PreferredSucc->BB) {
> > +        DEBUG(dbgs() << "ADCE: (Post)DomTree edge enqueued for deletion"
> > +                     << BB->getName() << " -> " << Succ->getName() <<
> "\n");
> > +        DeletedEdges.push_back({DominatorTree::Delete, BB, Succ});
> > +      }
> > +    }
> > +
> > +    DT.applyUpdates(DeletedEdges);
> > +    PDT.applyUpdates(DeletedEdges);
> > +
> >     NumBranchesRemoved += 1;
> >   }
> > }
> > @@ -609,6 +635,9 @@ void AggressiveDeadCodeElimination::make
> >   InstInfo[NewTerm].Live = true;
> >   if (const DILocation *DL = PredTerm->getDebugLoc())
> >     NewTerm->setDebugLoc(DL);
> > +
> > +  InstInfo.erase(PredTerm);
> > +  PredTerm->eraseFromParent();
> > }
> >
> > //===-------------------------------------------------------
> ---------------===//
> > @@ -617,13 +646,16 @@ void AggressiveDeadCodeElimination::make
> > //
> > //===-------------------------------------------------------
> ---------------===//
> > PreservedAnalyses ADCEPass::run(Function &F, FunctionAnalysisManager
> &FAM) {
> > +  auto &DT = FAM.getResult<DominatorTreeAnalysis>(F);
> >   auto &PDT = FAM.getResult<PostDominatorTreeAnalysis>(F);
> > -  if (!AggressiveDeadCodeElimination(F, PDT).
> performDeadCodeElimination())
> > +  if (!AggressiveDeadCodeElimination(F, DT, PDT).
> performDeadCodeElimination())
> >     return PreservedAnalyses::all();
> >
> >   PreservedAnalyses PA;
> >   PA.preserveSet<CFGAnalyses>();
> >   PA.preserve<GlobalsAA>();
> > +  PA.preserve<DominatorTreeAnalysis>();
> > +  PA.preserve<PostDominatorTreeAnalysis>();
> >   return PA;
> > }
> >
> > @@ -637,14 +669,23 @@ struct ADCELegacyPass : public FunctionP
> >   bool runOnFunction(Function &F) override {
> >     if (skipFunction(F))
> >       return false;
> > +
> > +    auto &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
> >     auto &PDT = getAnalysis<PostDominatorTreeWrapperPass>(
> ).getPostDomTree();
> > -    return AggressiveDeadCodeElimination(F, PDT).
> performDeadCodeElimination();
> > +    return AggressiveDeadCodeElimination(F, DT, PDT)
> > +        .performDeadCodeElimination();
> >   }
> >
> >   void getAnalysisUsage(AnalysisUsage &AU) const override {
> > +    // We require DominatorTree here only to update and thus preserve
> it.
> > +    AU.addRequired<DominatorTreeWrapperPass>();
> >     AU.addRequired<PostDominatorTreeWrapperPass>();
> >     if (!RemoveControlFlowFlag)
> >       AU.setPreservesCFG();
> > +    else {
> > +      AU.addPreserved<DominatorTreeWrapperPass>();
> > +      AU.addPreserved<PostDominatorTreeWrapperPass>();
> > +    }
> >     AU.addPreserved<GlobalsAAWrapperPass>();
> >   }
> > };
> > @@ -653,6 +694,7 @@ struct ADCELegacyPass : public FunctionP
> > char ADCELegacyPass::ID = 0;
> > INITIALIZE_PASS_BEGIN(ADCELegacyPass, "adce",
> >                       "Aggressive Dead Code Elimination", false, false)
> > +INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
> > INITIALIZE_PASS_DEPENDENCY(PostDominatorTreeWrapperPass)
> > INITIALIZE_PASS_END(ADCELegacyPass, "adce", "Aggressive Dead Code
> Elimination",
> >                     false, false)
> >
> > Added: llvm/trunk/test/Transforms/ADCE/2017-08-21-DomTree-deletions.ll
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/ADCE/2017-08-21-DomTree-deletions.ll?rev=311467&view=auto
> > ============================================================
> ==================
> > --- llvm/trunk/test/Transforms/ADCE/2017-08-21-DomTree-deletions.ll
> (added)
> > +++ llvm/trunk/test/Transforms/ADCE/2017-08-21-DomTree-deletions.ll Tue
> Aug 22 09:30:21 2017
> > @@ -0,0 +1,24 @@
> > +; RUN: opt < %s -adce | llvm-dis
> > +; RUN: opt < %s -adce -verify-dom-info | llvm-dis
> > +
> > +define void @foo() {
> > +entry:
> > +  br label %switch
> > +switch:                    ; preds = %entry
> > +  switch i32 undef, label %default [
> > +    i32 2, label %two
> > +    i32 5, label %five
> > +    i32 4, label %four
> > +  ]
> > +four:                      ; preds = %switch
> > +  br label %exit
> > +five:                      ; preds = %switch
> > +  br label %exit
> > +two:                       ; preds = %switch
> > +  br label %exit
> > +default:                   ; preds = %switch
> > +  br label %exit
> > +exit:                      ; preds = %default, %two, %five, %four
> > +  ret void
> > +}
> > +
> >
> > Added: llvm/trunk/test/Transforms/ADCE/domtree-DoubleDeletion.ll
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/ADCE/domtree-DoubleDeletion.ll?rev=311467&view=auto
> > ============================================================
> ==================
> > --- llvm/trunk/test/Transforms/ADCE/domtree-DoubleDeletion.ll (added)
> > +++ llvm/trunk/test/Transforms/ADCE/domtree-DoubleDeletion.ll Tue Aug
> 22 09:30:21 2017
> > @@ -0,0 +1,39 @@
> > +; RUN: opt < %s -gvn -simplifycfg -adce | llvm-dis
> > +; RUN: opt < %s -gvn -simplifycfg -adce -verify-dom-info | llvm-dis
> > +
> > +; This test makes sure that the DominatorTree properly handles
> > +; deletion of edges that go to forward-unreachable regions.
> > +; In this case, %land.end is already forward unreachable when
> > +; the DT gets informed about the deletion of %entry -> %land.end.
> > +
> > + at a = common global i32 0, align 4
> > +
> > +define i32 @main() {
> > +entry:
> > +  %retval = alloca i32, align 4
> > +  store i32 0, i32* %retval, align 4
> > +  %0 = load i32, i32* @a, align 4
> > +  %cmp = icmp ne i32 %0, 1
> > +  br i1 %cmp, label %land.rhs, label %land.end4
> > +
> > +land.rhs:                                         ; preds = %entry
> > +  %1 = load i32, i32* @a, align 4
> > +  %tobool = icmp ne i32 %1, 0
> > +  br i1 %tobool, label %land.rhs1, label %land.end
> > +
> > +land.rhs1:                                        ; preds = %land.rhs
> > +  br label %land.end
> > +
> > +land.end:                                         ; preds = %land.rhs1,
> %land.rhs
> > +  %2 = phi i1 [ false, %land.rhs ], [ true, %land.rhs1 ]
> > +  %land.ext = zext i1 %2 to i32
> > +  %conv = trunc i32 %land.ext to i16
> > +  %conv2 = sext i16 %conv to i32
> > +  %tobool3 = icmp ne i32 %conv2, 0
> > +  br label %land.end4
> > +
> > +land.end4:                                        ; preds = %land.end,
> %entry
> > +  %3 = phi i1 [ false, %entry ], [ %tobool3, %land.end ]
> > +  %land.ext5 = zext i1 %3 to i32
> > +  ret i32 0
> > +}
> >
> > Added: llvm/trunk/test/Transforms/ADCE/unreachable.ll
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/ADCE/unreachable.ll?rev=311467&view=auto
> > ============================================================
> ==================
> > --- llvm/trunk/test/Transforms/ADCE/unreachable.ll (added)
> > +++ llvm/trunk/test/Transforms/ADCE/unreachable.ll Tue Aug 22 09:30:21
> 2017
> > @@ -0,0 +1,18 @@
> > +; RUN: opt < %s -adce -simplifycfg | llvm-dis
> > +; RUN: opt < %s -passes=adce | llvm-dis
> > +
> > +define i32 @Test(i32 %A, i32 %B) {
> > +BB1:
> > +        br label %BB4
> > +
> > +BB2:            ; No predecessors!
> > +        br label %BB3
> > +
> > +BB3:            ; preds = %BB4, %BB2
> > +        %ret = phi i32 [ %X, %BB4 ], [ %B, %BB2 ]               ; <i32>
> [#uses=1]
> > +        ret i32 %ret
> > +
> > +BB4:            ; preds = %BB1
> > +        %X = phi i32 [ %A, %BB1 ]               ; <i32> [#uses=1]
> > +        br label %BB3
> > +}
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://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/20170823/27d5c523/attachment.html>


More information about the llvm-commits mailing list