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

Adrian Prantl via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 23 11:17:55 PDT 2017


> On Aug 23, 2017, at 11:05 AM, Jakub (Kuba) Kuderski <kubakuderski at gmail.com> wrote:
> 
> > Is that something that changed recently? Do you know when?
> 
> It was introduced quite some time ago, but the difference is that the DominatorTree wasn't preserved before this patch, and the verification is run in verifyPreservedAnalyses.
> 
> > It depends on what the other options are. If the new, more expensive checks cannot be improved to perform better, we should decide whether it's worth the extra resources on the builders.
> 
> One obvious choice would be to run more expensive verifiers only when -verify-dom-info is explicitly set. Do you want me to go ahead and make a patch for that?

So to summarize:
- we previously didn't run DomTree verifier because the analysis was not preserved
- your patch changed it to be preserved, so now we run the verifier, but it is very expensive

Our options are:
1. Hide the DomTree verifier behind another option and don't run it when expensive checks are enabled
2. Increase the timeout on the bot and look into improving the performance of the verifier

How much value do you think the verification adds?
Option (1) would preserve the status quo, but it seems awkward to me to have a verifier that is so expensive that it's disabled in the expensive checks.

What puzzles me at the moment is how inconsistent the runtime is and I would like to get to the bottom of this before we reach a decision.
+Mike, are green-dragon-13 and green-dragon-09 identical hardware? If not, does the difference in hardware explain that green-dragon-13 takes +1h for the same job?

Jakub, is it possible that the verifier is behaving nondeterministically?

-- adrian
 
> 
> On Aug 23, 2017 19:58, "Adrian Prantl" <aprantl at apple.com> wrote:
> 
>> On Aug 23, 2017, at 10:38 AM, Jakub (Kuba) Kuderski <kubakuderski at gmail.com> wrote:
>> 
>> Hi Adrian,
>> 
>> Expensive checks make .verifyDominatorTree to run O(n^3) sibling property verification, which can cause some major slowndowns on big CFG's.
> 
> Is that something that changed recently? Do you know when?
> 
>> 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.
> 
> It depends on what the other options are. If the new, more expensive checks cannot be improved to perform better, we should decide whether it's worth the extra resources on the builders.
> 
> -- adrian
>> 
>> 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
>> 
> 
> 



More information about the llvm-commits mailing list