<div dir="ltr">Hi Daniel,<br><br>Thanks for reporting the bug.<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px">Would I be right in thinking that TreeNodePtr::getLevel() returns the depth in the dominator tree?</span></blockquote><div>Correct.<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span style="font-size:12.8px">If so, I think that the update to indicate that %four dominates %exit when the %switch->%default path is deleted is causing %exit to be misidentified as unreachable in the later updates because DescendAndCollect() is only checking the depth of the successors it finds. If %exit had been updated after all the removals then it would have turned out ok.</span></blockquote><div>I don't have any recent LLVM revision on my machine at the moment, so it's a bit difficult for me to follow just the description. I'll take a look at it tomorrow. In the meanwhile, feel free to revert the ADCE patch.<br><br>Best,<br>Kuba</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 21, 2017 at 7:11 PM, Daniel Sanders <span dir="ltr"><<a href="mailto:daniel_l_sanders@apple.com" target="_blank">daniel_l_sanders@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space">Hi,<div><br></div><div>I'm currently seeing a crash in ADCE which I've narrowed down to this commit. Given this bugpoint-reduced test case:</div><div><font face="Courier New">define void @foo() {<br>entry:<br> br label %switch<br>switch: ; preds = %entry<br> switch i32 undef, label %<wbr>default [<br> i32 2, label %two<br> i32 5, label %five<br> i32 4, label %four<br> ]<br>four: ; preds = %switch<br> br label %exit<br>five: ; preds = %switch<br> br label %exit<br>two: ; preds = %switch<br> br label %exit<br>default: ; preds = %switch<br> br label %exit<br>exit: ; preds = %default, %two, %five, %four<br> ret void<br>}</font></div><div>the command '<font face="Courier New">./bin/opt bugpoint-reduced-simplified.ll -adce -S -o -</font>' asserts with:</div><div><font face="Courier New">Assertion failed: (TN), function operator(), file ../llvm/include/llvm/Support/<wbr>GenericDomTreeConstruction.h, <wbr>line 1031.<br>Stack dump:<br>0.<span class="m_7556218827904289853Apple-tab-span" style="white-space:pre-wrap"> </span>Program arguments: ./bin/opt bugpoint-reduced-simplified.ll -adce -S -o - <br>1.<span class="m_7556218827904289853Apple-tab-span" style="white-space:pre-wrap"> </span>Running pass 'Function Pass Manager' on module 'bugpoint-reduced-simplified.<wbr>ll'.<br>2.<span class="m_7556218827904289853Apple-tab-span" style="white-space:pre-wrap"> </span>Running pass 'Aggressive Dead Code Elimination' on function '@foo'<br>Abort trap: 6</font><br><br></div><div>According to the -debug output and some additional instrumentation, ADCE starts by re-writing the switch to a 'br label %four'. I was expecting it to choose %default but all choices are equivalent so it doesn't matter much. At this point it deletes the %switch->%default CFG edge, erases %default, and updates the dominator tree to indicate that %four dominates %exit. It then deletes the %switch->%two CFG edge and it's at this point that things start to go wrong.</div><div><br></div><div>After deleting the %switch->%two edge it erases %two but it also erases %exit which is still being used by %four (as well as %default, %two, and %five). The dominator tree is correctly updated to reflect the removal of both %two and %exit. It then proceeds to delete the %switch->%five edge and hits the assertion when it attempts to look up a TreeNodePtr for %exit and doesn't find one.</div><div><br></div><div>I'm currently thinking that this is a processing order issue. Would I be right in thinking that TreeNodePtr::getLevel() returns the depth in the dominator tree? If so, I think that the update to indicate that %four dominates %exit when the %switch->%default path is deleted is causing %exit to be misidentified as unreachable in the later updates because DescendAndCollect() is only checking the depth of the successors it finds. If %exit had been updated after all the removals then it would have turned out ok.</div><div><div class="h5"><div><br><div><blockquote type="cite"><div>On 17 Aug 2017, at 02:41, Jakub Kuderski via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="m_7556218827904289853Apple-interchange-newline"><div><div>Author: kuhar<br>Date: Wed Aug 16 18:41:49 2017<br>New Revision: 311057<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=311057&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=311057&view=rev</a><br>Log:<br>Reapply: [ADCE][Dominators] Teach ADCE to preserve dominators<br><br>Summary:<br>This patch teaches ADCE to preserve both DominatorTrees and PostDominatorTrees.<br><br>I didn't notice any performance impact when bootstrapping clang with this patch.<br><br>The patch was originally committed in r311039 and reverted in r311049.<br>This revision fixes the problem with not adding a dependency on the<br>DominatorTreeWrapperPass for the LegacyPassManager.<br><br>Reviewers: dberlin, chandlerc, sanjoy, davide, grosser, brzycki<br><br>Reviewed By: davide<br><br>Subscribers: grandinj, zhendongsu, llvm-commits, david2050<br><br>Differential Revision: <a href="https://reviews.llvm.org/D35869" target="_blank">https://reviews.llvm.org/<wbr>D35869</a><br><br>Added:<br> llvm/trunk/test/Transforms/<wbr>ADCE/domtree-DoubleDeletion.ll<br> llvm/trunk/test/Transforms/<wbr>ADCE/unreachable.ll<br>Modified:<br> llvm/trunk/include/llvm/<wbr>Support/<wbr>GenericDomTreeConstruction.h<br> llvm/trunk/lib/Transforms/<wbr>Scalar/ADCE.cpp<br><br>Modified: llvm/trunk/include/llvm/<wbr>Support/<wbr>GenericDomTreeConstruction.h<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h?rev=311057&r1=311056&r2=311057&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/Support/<wbr>GenericDomTreeConstruction.h?<wbr>rev=311057&r1=311056&r2=<wbr>311057&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- llvm/trunk/include/llvm/<wbr>Support/<wbr>GenericDomTreeConstruction.h (original)<br>+++ llvm/trunk/include/llvm/<wbr>Support/<wbr>GenericDomTreeConstruction.h Wed Aug 16 18:41:49 2017<br>@@ -914,7 +914,12 @@ struct SemiNCAInfo {<br> if (!FromTN) return;<br><br> const TreeNodePtr ToTN = DT.getNode(To);<br>- assert(ToTN && "To already unreachable -- there is no edge to delete");<br>+ if (!ToTN) {<br>+ DEBUG(dbgs() << "\tTo (" << BlockNamePrinter(To)<br>+ << ") already unreachable -- there is no edge to delete\n");<br>+ return;<br>+ }<br>+<br> const NodePtr NCDBlock = DT.findNearestCommonDominator(<wbr>From, To);<br> const TreeNodePtr NCD = DT.getNode(NCDBlock);<br><br><br>Modified: llvm/trunk/lib/Transforms/<wbr>Scalar/ADCE.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/ADCE.cpp?rev=311057&r1=311056&r2=311057&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/Scalar/ADCE.cpp?<wbr>rev=311057&r1=311056&r2=<wbr>311057&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- llvm/trunk/lib/Transforms/<wbr>Scalar/ADCE.cpp (original)<br>+++ llvm/trunk/lib/Transforms/<wbr>Scalar/ADCE.cpp Wed Aug 16 18:41:49 2017<br>@@ -27,6 +27,7 @@<br> #include "llvm/IR/BasicBlock.h"<br> #include "llvm/IR/CFG.h"<br> #include "llvm/IR/DebugInfoMetadata.h"<br>+#include "llvm/IR/Dominators.h"<br> #include "llvm/IR/IRBuilder.h"<br> #include "llvm/IR/InstIterator.h"<br> #include "llvm/IR/Instructions.h"<br>@@ -89,6 +90,10 @@ struct BlockInfoType {<br><br> class AggressiveDeadCodeElimination {<br> Function &F;<br>+<br>+ // ADCE does not use DominatorTree per se, but it updates it to preserve the<br>+ // analysis.<br>+ DominatorTree &DT;<br> PostDominatorTree &PDT;<br><br> /// Mapping of blocks to associated information, an element in BlockInfoVec.<br>@@ -157,9 +162,10 @@ class AggressiveDeadCodeElimination {<br> void makeUnconditional(BasicBlock *BB, BasicBlock *Target);<br><br> public:<br>- <wbr>AggressiveDeadCodeElimination(<wbr>Function &F, PostDominatorTree &PDT)<br>- : F(F), PDT(PDT) {}<br>- bool performDeadCodeElimination();<br>+ AggressiveDeadCodeElimination(<wbr>Function &F, DominatorTree &DT,<br>+ <wbr>PostDominatorTree &PDT)<br>+ : F(F), DT(DT), PDT(PDT) {}<br>+ bool performDeadCodeElimination();<br> };<br> }<br><br>@@ -557,14 +563,31 @@ void AggressiveDeadCodeElimination:<wbr>:upda<br> }<br> assert((PreferredSucc && PreferredSucc->PostOrder > 0) &&<br> "Failed to find safe successor for dead branch");<br>+<br>+ // Collect removed successors to update the (Post)DominatorTrees.<br>+ SmallPtrSet<BasicBlock *, 4> RemovedSuccessors;<br> bool First = true;<br> for (auto *Succ : successors(BB)) {<br>- if (!First || Succ != PreferredSucc->BB)<br>+ if (!First || Succ != PreferredSucc->BB) {<br> Succ-><wbr>removePredecessor(BB);<br>- else<br>+ RemovedSuccessors.<wbr>insert(Succ);<br>+ } else<br> First = false;<br> }<br> makeUnconditional(BB, PreferredSucc->BB);<br>+<br>+ // Inform the dominators about the deleted CFG edges.<br>+ for (auto *Succ : RemovedSuccessors) {<br>+ // It might have happened that the same successor appeared multiple times<br>+ // and the CFG edge wasn't really removed.<br>+ if (Succ != PreferredSucc->BB) {<br>+ DEBUG(dbgs() << "ADCE: Removing (Post)DomTree edge " << BB->getName()<br>+ << " -> " << Succ->getName() << "\n");<br>+ DT.deleteEdge(BB, Succ);<br>+ PDT.deleteEdge(BB, Succ);<br>+ }<br>+ }<br>+<br> NumBranchesRemoved += 1;<br> }<br> }<br>@@ -609,6 +632,9 @@ void AggressiveDeadCodeElimination:<wbr>:make<br> InstInfo[NewTerm].Live = true;<br> if (const DILocation *DL = PredTerm->getDebugLoc())<br> NewTerm->setDebugLoc(DL);<br>+<br>+ InstInfo.erase(PredTerm);<br>+ PredTerm->eraseFromParent();<br> }<br><br> //===-------------------------<wbr>------------------------------<wbr>---------------===//<br>@@ -617,13 +643,16 @@ void AggressiveDeadCodeElimination:<wbr>:make<br> //<br> //===-------------------------<wbr>------------------------------<wbr>---------------===//<br> PreservedAnalyses ADCEPass::run(Function &F, FunctionAnalysisManager &FAM) {<br>+ auto &DT = FAM.getResult<<wbr>DominatorTreeAnalysis>(F);<br> auto &PDT = FAM.getResult<<wbr>PostDominatorTreeAnalysis>(F);<br>- if (!<wbr>AggressiveDeadCodeElimination(<wbr>F, PDT).<wbr>performDeadCodeElimination())<br>+ if (!<wbr>AggressiveDeadCodeElimination(<wbr>F, DT, PDT).<wbr>performDeadCodeElimination())<br> return PreservedAnalyses::all();<br><br> PreservedAnalyses PA;<br> PA.preserveSet<CFGAnalyses>(<wbr>);<br> PA.preserve<GlobalsAA>();<br>+ PA.preserve<<wbr>DominatorTreeAnalysis>();<br>+ PA.preserve<<wbr>PostDominatorTreeAnalysis>();<br> return PA;<br> }<br><br>@@ -637,14 +666,23 @@ struct ADCELegacyPass : public FunctionP<br> bool runOnFunction(Function &F) override {<br> if (skipFunction(F))<br> return false;<br>+<br>+ auto &DT = getAnalysis<<wbr>DominatorTreeWrapperPass>().<wbr>getDomTree();<br> auto &PDT = getAnalysis<<wbr>PostDominatorTreeWrapperPass>(<wbr>).getPostDomTree();<br>- return AggressiveDeadCodeElimination(<wbr>F, PDT).<wbr>performDeadCodeElimination();<br>+ return AggressiveDeadCodeElimination(<wbr>F, DT, PDT)<br>+ .<wbr>performDeadCodeElimination();<br> }<br><br> void getAnalysisUsage(AnalysisUsage &AU) const override {<br>+ // We require DominatorTree here only to update and thus preserve it.<br>+ AU.addRequired<<wbr>DominatorTreeWrapperPass>();<br> AU.addRequired<<wbr>PostDominatorTreeWrapperPass>(<wbr>);<br> if (!RemoveControlFlowFlag)<br> AU.setPreservesCFG();<br>+ else {<br>+ AU.addPreserved<<wbr>DominatorTreeWrapperPass>();<br>+ AU.addPreserved<<wbr>PostDominatorTreeWrapperPass>(<wbr>);<br>+ }<br> AU.addPreserved<<wbr>GlobalsAAWrapperPass>();<br> }<br> };<br>@@ -653,6 +691,7 @@ struct ADCELegacyPass : public FunctionP<br> char ADCELegacyPass::ID = 0;<br> INITIALIZE_PASS_BEGIN(<wbr>ADCELegacyPass, "adce",<br> "<wbr>Aggressive Dead Code Elimination", false, false)<br>+INITIALIZE_PASS_DEPENDENCY(<wbr>DominatorTreeWrapperPass)<br> INITIALIZE_PASS_DEPENDENCY(<wbr>PostDominatorTreeWrapperPass)<br> INITIALIZE_PASS_END(<wbr>ADCELegacyPass, "adce", "Aggressive Dead Code Elimination",<br> false, false)<br><br>Added: llvm/trunk/test/Transforms/<wbr>ADCE/domtree-DoubleDeletion.ll<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ADCE/domtree-DoubleDeletion.ll?rev=311057&view=auto" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/ADCE/domtree-<wbr>DoubleDeletion.ll?rev=311057&<wbr>view=auto</a><br>==============================<wbr>==============================<wbr>==================<br>--- llvm/trunk/test/Transforms/<wbr>ADCE/domtree-DoubleDeletion.ll (added)<br>+++ llvm/trunk/test/Transforms/<wbr>ADCE/domtree-DoubleDeletion.ll Wed Aug 16 18:41:49 2017<br>@@ -0,0 +1,39 @@<br>+; RUN: opt < %s -gvn -simplifycfg -adce | llvm-dis<br>+; RUN: opt < %s -gvn -simplifycfg -adce -verify-dom-info | llvm-dis<br>+<br>+; This test makes sure that the DominatorTree properly handles<br>+; deletion of edges that go to forward-unreachable regions.<br>+; In this case, %land.end is already forward unreachable when<br>+; the DT gets informed about the deletion of %entry -> %land.end.<br>+<br>+@a = common global i32 0, align 4<br>+<br>+define i32 @main() {<br>+entry:<br>+ %retval = alloca i32, align 4<br>+ store i32 0, i32* %retval, align 4<br>+ %0 = load i32, i32* @a, align 4<br>+ %cmp = icmp ne i32 %0, 1<br>+ br i1 %cmp, label %land.rhs, label %land.end4<br>+<br>+land.rhs: <wbr> ; preds = %entry<br>+ %1 = load i32, i32* @a, align 4<br>+ %tobool = icmp ne i32 %1, 0<br>+ br i1 %tobool, label %land.rhs1, label %land.end<br>+<br>+land.rhs1: <wbr> ; preds = %land.rhs<br>+ br label %land.end<br>+<br>+land.end: <wbr> ; preds = %land.rhs1, %land.rhs<br>+ %2 = phi i1 [ false, %land.rhs ], [ true, %land.rhs1 ]<br>+ %land.ext = zext i1 %2 to i32<br>+ %conv = trunc i32 %land.ext to i16<br>+ %conv2 = sext i16 %conv to i32<br>+ %tobool3 = icmp ne i32 %conv2, 0<br>+ br label %land.end4<br>+<br>+land.end4: <wbr> ; preds = %land.end, %entry<br>+ %3 = phi i1 [ false, %entry ], [ %tobool3, %land.end ]<br>+ %land.ext5 = zext i1 %3 to i32<br>+ ret i32 0<br>+}<br><br>Added: llvm/trunk/test/Transforms/<wbr>ADCE/unreachable.ll<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ADCE/unreachable.ll?rev=311057&view=auto" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/ADCE/unreachable.<wbr>ll?rev=311057&view=auto</a><br>==============================<wbr>==============================<wbr>==================<br>--- llvm/trunk/test/Transforms/<wbr>ADCE/unreachable.ll (added)<br>+++ llvm/trunk/test/Transforms/<wbr>ADCE/unreachable.ll Wed Aug 16 18:41:49 2017<br>@@ -0,0 +1,18 @@<br>+; RUN: opt < %s -adce -simplifycfg | llvm-dis<br>+; RUN: opt < %s -passes=adce | llvm-dis<br>+<br>+define i32 @Test(i32 %A, i32 %B) {<br>+BB1:<br>+ br label %BB4<br>+<br>+BB2: ; No predecessors!<br>+ br label %BB3<br>+<br>+BB3: ; preds = %BB4, %BB2<br>+ %ret = phi i32 [ %X, %BB4 ], [ %B, %BB2 ] ; <i32> [#uses=1]<br>+ ret i32 %ret<br>+<br>+BB4: ; preds = %BB1<br>+ %X = phi i32 [ %A, %BB1 ] ; <i32> [#uses=1]<br>+ br label %BB3<br>+}<br><br><br>______________________________<wbr>_________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br></div></div></blockquote></div><br></div></div></div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div>Jakub Kuderski</div></div>
</div>