<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Thanks<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 21 Aug 2017, at 22:01, Jakub (Kuba) Kuderski <<a href="mailto:kubakuderski@gmail.com" class="">kubakuderski@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">This patch is temporarily reverted by <span style="white-space: pre-wrap;" class="">r311381.</span><div class="gmail_extra"><br class=""><div class="gmail_quote">On Mon, Aug 21, 2017 at 7:24 PM, Jakub (Kuba) Kuderski <span dir="ltr" class=""><<a href="mailto:kubakuderski@gmail.com" target="_blank" class="">kubakuderski@gmail.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="">Hi Daniel,<br class=""><br class="">Thanks for reporting the bug.<span class=""><br class=""><br class=""><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" class="">Would I be right in thinking that TreeNodePtr::getLevel() returns the depth in the dominator tree?</span></blockquote></span><div class="">Correct.<span class=""><br class=""><br class=""><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" class="">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></span><div class="">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 class=""><br class="">Best,<br class="">Kuba</div></div></div><div class="gmail_extra"><div class=""><div class="m_-1183188594685633740h5"><br class=""><div class="gmail_quote">On Mon, Aug 21, 2017 at 7:11 PM, Daniel Sanders <span dir="ltr" class=""><<a href="mailto:daniel_l_sanders@apple.com" target="_blank" class="">daniel_l_sanders@apple.com</a>></span> wrote:<br class=""><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" class="">Hi,<div class=""><br class=""></div><div class="">I'm currently seeing a crash in ADCE which I've narrowed down to this commit. Given this bugpoint-reduced test case:</div><div class=""><font face="Courier New" class="">define void @foo() {<br class="">entry:<br class=""> br label %switch<br class="">switch: ; preds = %entry<br class=""> switch i32 undef, label %def<wbr class="">ault [<br class=""> i32 2, label %two<br class=""> i32 5, label %five<br class=""> i32 4, label %four<br class=""> ]<br class="">four: ; preds = %switch<br class=""> br label %exit<br class="">five: ; preds = %switch<br class=""> br label %exit<br class="">two: ; preds = %switch<br class=""> br label %exit<br class="">default: ; preds = %switch<br class=""> br label %exit<br class="">exit: ; preds = %default, %two, %five, %four<br class=""> ret void<br class="">}</font></div><div class="">the command '<font face="Courier New" class="">./bin/opt bugpoint-reduced-simplified.ll -adce -S -o -</font>' asserts with:</div><div class=""><font face="Courier New" class="">Assertion failed: (TN), function operator(), file ../llvm/include/llvm/Support/G<wbr class="">enericDomTreeConstruction.h, l<wbr class="">ine 1031.<br class="">Stack dump:<br class="">0.<span class="m_-1183188594685633740m_3621188403423764731m_7556218827904289853Apple-tab-span" style="white-space:pre-wrap"> </span>Program arguments: ./bin/opt bugpoint-reduced-simplified.ll -adce -S -o - <br class="">1.<span class="m_-1183188594685633740m_3621188403423764731m_7556218827904289853Apple-tab-span" style="white-space:pre-wrap"> </span>Running pass 'Function Pass Manager' on module 'bugpoint-reduced-simplified.l<wbr class="">l'.<br class="">2.<span class="m_-1183188594685633740m_3621188403423764731m_7556218827904289853Apple-tab-span" style="white-space:pre-wrap"> </span>Running pass 'Aggressive Dead Code Elimination' on function '@foo'<br class="">Abort trap: 6</font><br class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class=""><div class="m_-1183188594685633740m_3621188403423764731h5"><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On 17 Aug 2017, at 02:41, Jakub Kuderski via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="m_-1183188594685633740m_3621188403423764731m_7556218827904289853Apple-interchange-newline"><div class=""><div class="">Author: kuhar<br class="">Date: Wed Aug 16 18:41:49 2017<br class="">New Revision: 311057<br class=""><br class="">URL: <a href="http://llvm.org/viewvc/llvm-project?rev=311057&view=rev" target="_blank" class="">http://llvm.org/viewvc/llvm-pr<wbr class="">oject?rev=311057&view=rev</a><br class="">Log:<br class="">Reapply: [ADCE][Dominators] Teach ADCE to preserve dominators<br class=""><br class="">Summary:<br class="">This patch teaches ADCE to preserve both DominatorTrees and PostDominatorTrees.<br class=""><br class="">I didn't notice any performance impact when bootstrapping clang with this patch.<br class=""><br class="">The patch was originally committed in r311039 and reverted in r311049.<br class="">This revision fixes the problem with not adding a dependency on the<br class="">DominatorTreeWrapperPass for the LegacyPassManager.<br class=""><br class="">Reviewers: dberlin, chandlerc, sanjoy, davide, grosser, brzycki<br class=""><br class="">Reviewed By: davide<br class=""><br class="">Subscribers: grandinj, zhendongsu, llvm-commits, david2050<br class=""><br class="">Differential Revision: <a href="https://reviews.llvm.org/D35869" target="_blank" class="">https://reviews.llvm.org/D3586<wbr class="">9</a><br class=""><br class="">Added:<br class=""> llvm/trunk/test/Transforms/<wbr class="">ADCE/domtree-DoubleDeletion.ll<br class=""> llvm/trunk/test/Transforms/<wbr class="">ADCE/unreachable.ll<br class="">Modified:<br class=""> llvm/trunk/include/llvm/Sup<wbr class="">port/GenericDomTreeConstructio<wbr class="">n.h<br class=""> llvm/trunk/lib/Transforms/S<wbr class="">calar/ADCE.cpp<br class=""><br class="">Modified: llvm/trunk/include/llvm/Suppor<wbr class="">t/GenericDomTreeConstruction.h<br class="">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" class="">http://llvm.org/viewvc/llvm-pr<wbr class="">oject/llvm/trunk/include/llvm/<wbr class="">Support/GenericDomTreeConstruc<wbr class="">tion.h?rev=311057&r1=311056&r2<wbr class="">=311057&view=diff</a><br class="">==============================<wbr class="">==============================<wbr class="">==================<br class="">--- llvm/trunk/include/llvm/Suppor<wbr class="">t/GenericDomTreeConstruction.h (original)<br class="">+++ llvm/trunk/include/llvm/Suppor<wbr class="">t/GenericDomTreeConstruction.h Wed Aug 16 18:41:49 2017<br class="">@@ -914,7 +914,12 @@ struct SemiNCAInfo {<br class=""> if (!FromTN) return;<br class=""><br class=""> const TreeNodePtr ToTN = DT.getNode(To);<br class="">- assert(ToTN && "To already unreachable -- there is no edge to delete");<br class="">+ if (!ToTN) {<br class="">+ DEBUG(dbgs() << "\tTo (" << BlockNamePrinter(To)<br class="">+ << ") already unreachable -- there is no edge to delete\n");<br class="">+ return;<br class="">+ }<br class="">+<br class=""> const NodePtr NCDBlock = DT.findNearestCommonDominator(<wbr class="">From, To);<br class=""> const TreeNodePtr NCD = DT.getNode(NCDBlock);<br class=""><br class=""><br class="">Modified: llvm/trunk/lib/Transforms/Scal<wbr class="">ar/ADCE.cpp<br class="">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" class="">http://llvm.org/viewvc/llvm-pr<wbr class="">oject/llvm/trunk/lib/Transform<wbr class="">s/Scalar/ADCE.cpp?rev=311057&r<wbr class="">1=311056&r2=311057&view=diff</a><br class="">==============================<wbr class="">==============================<wbr class="">==================<br class="">--- llvm/trunk/lib/Transforms/Scal<wbr class="">ar/ADCE.cpp (original)<br class="">+++ llvm/trunk/lib/Transforms/Scal<wbr class="">ar/ADCE.cpp Wed Aug 16 18:41:49 2017<br class="">@@ -27,6 +27,7 @@<br class=""> #include "llvm/IR/BasicBlock.h"<br class=""> #include "llvm/IR/CFG.h"<br class=""> #include "llvm/IR/DebugInfoMetadata.h"<br class="">+#include "llvm/IR/Dominators.h"<br class=""> #include "llvm/IR/IRBuilder.h"<br class=""> #include "llvm/IR/InstIterator.h"<br class=""> #include "llvm/IR/Instructions.h"<br class="">@@ -89,6 +90,10 @@ struct BlockInfoType {<br class=""><br class=""> class AggressiveDeadCodeElimination {<br class=""> Function &F;<br class="">+<br class="">+ // ADCE does not use DominatorTree per se, but it updates it to preserve the<br class="">+ // analysis.<br class="">+ DominatorTree &DT;<br class=""> PostDominatorTree &PDT;<br class=""><br class=""> /// Mapping of blocks to associated information, an element in BlockInfoVec.<br class="">@@ -157,9 +162,10 @@ class AggressiveDeadCodeElimination {<br class=""> void makeUnconditional(BasicBlock *BB, BasicBlock *Target);<br class=""><br class=""> public:<br class="">- AggressiveDeadCodeElimination<wbr class="">(Function &F, PostDominatorTree &PDT)<br class="">- : F(F), PDT(PDT) {}<br class="">- bool performDeadCodeElimination();<br class="">+ AggressiveDeadCodeElimination(<wbr class="">Function &F, DominatorTree &DT,<br class="">+ <wbr class="">PostDominatorTree &PDT)<br class="">+ : F(F), DT(DT), PDT(PDT) {}<br class="">+ bool performDeadCodeElimination();<br class=""> };<br class=""> }<br class=""><br class="">@@ -557,14 +563,31 @@ void AggressiveDeadCodeElimination:<wbr class="">:upda<br class=""> }<br class=""> assert((PreferredSucc && PreferredSucc->PostOrder > 0) &&<br class=""> "Failed to find safe successor for dead branch");<br class="">+<br class="">+ // Collect removed successors to update the (Post)DominatorTrees.<br class="">+ SmallPtrSet<BasicBlock *, 4> RemovedSuccessors;<br class=""> bool First = true;<br class=""> for (auto *Succ : successors(BB)) {<br class="">- if (!First || Succ != PreferredSucc->BB)<br class="">+ if (!First || Succ != PreferredSucc->BB) {<br class=""> Succ->removePredecesso<wbr class="">r(BB);<br class="">- else<br class="">+ RemovedSuccessors.inser<wbr class="">t(Succ);<br class="">+ } else<br class=""> First = false;<br class=""> }<br class=""> makeUnconditional(BB, PreferredSucc->BB);<br class="">+<br class="">+ // Inform the dominators about the deleted CFG edges.<br class="">+ for (auto *Succ : RemovedSuccessors) {<br class="">+ // It might have happened that the same successor appeared multiple times<br class="">+ // and the CFG edge wasn't really removed.<br class="">+ if (Succ != PreferredSucc->BB) {<br class="">+ DEBUG(dbgs() << "ADCE: Removing (Post)DomTree edge " << BB->getName()<br class="">+ << " -> " << Succ->getName() << "\n");<br class="">+ DT.deleteEdge(BB, Succ);<br class="">+ PDT.deleteEdge(BB, Succ);<br class="">+ }<br class="">+ }<br class="">+<br class=""> NumBranchesRemoved += 1;<br class=""> }<br class=""> }<br class="">@@ -609,6 +632,9 @@ void AggressiveDeadCodeElimination:<wbr class="">:make<br class=""> InstInfo[NewTerm].Live = true;<br class=""> if (const DILocation *DL = PredTerm->getDebugLoc())<br class=""> NewTerm->setDebugLoc(DL);<br class="">+<br class="">+ InstInfo.erase(PredTerm);<br class="">+ PredTerm->eraseFromParent();<br class=""> }<br class=""><br class=""> //===-------------------------<wbr class="">------------------------------<wbr class="">---------------===//<br class="">@@ -617,13 +643,16 @@ void AggressiveDeadCodeElimination:<wbr class="">:make<br class=""> //<br class=""> //===-------------------------<wbr class="">------------------------------<wbr class="">---------------===//<br class=""> PreservedAnalyses ADCEPass::run(Function &F, FunctionAnalysisManager &FAM) {<br class="">+ auto &DT = FAM.getResult<DominatorTreeAna<wbr class="">lysis>(F);<br class=""> auto &PDT = FAM.getResult<PostDominatorTre<wbr class="">eAnalysis>(F);<br class="">- if (!AggressiveDeadCodeEliminatio<wbr class="">n(F, PDT).performDeadCodeEliminatio<wbr class="">n())<br class="">+ if (!AggressiveDeadCodeEliminatio<wbr class="">n(F, DT, PDT).performDeadCodeEliminatio<wbr class="">n())<br class=""> return PreservedAnalyses::all();<br class=""><br class=""> PreservedAnalyses PA;<br class=""> PA.preserveSet<CFGAnalyses>(<wbr class="">);<br class=""> PA.preserve<GlobalsAA>();<br class="">+ PA.preserve<DominatorTreeAnal<wbr class="">ysis>();<br class="">+ PA.preserve<PostDominatorTree<wbr class="">Analysis>();<br class=""> return PA;<br class=""> }<br class=""><br class="">@@ -637,14 +666,23 @@ struct ADCELegacyPass : public FunctionP<br class=""> bool runOnFunction(Function &F) override {<br class=""> if (skipFunction(F))<br class=""> return false;<br class="">+<br class="">+ auto &DT = getAnalysis<DominatorTreeWrapp<wbr class="">erPass>().getDomTree();<br class=""> auto &PDT = getAnalysis<PostDominatorTreeW<wbr class="">rapperPass>().getPostDomTree()<wbr class="">;<br class="">- return AggressiveDeadCodeElimination(<wbr class="">F, PDT).performDeadCodeEliminatio<wbr class="">n();<br class="">+ return AggressiveDeadCodeElimination(<wbr class="">F, DT, PDT)<br class="">+ .performDeadCodeElimina<wbr class="">tion();<br class=""> }<br class=""><br class=""> void getAnalysisUsage(AnalysisUsage &AU) const override {<br class="">+ // We require DominatorTree here only to update and thus preserve it.<br class="">+ AU.addRequired<DominatorTre<wbr class="">eWrapperPass>();<br class=""> AU.addRequired<PostDominat<wbr class="">orTreeWrapperPass>();<br class=""> if (!RemoveControlFlowFlag)<br class=""> AU.setPreservesCFG();<br class="">+ else {<br class="">+ AU.addPreserved<Dominator<wbr class="">TreeWrapperPass>();<br class="">+ AU.addPreserved<PostDomin<wbr class="">atorTreeWrapperPass>();<br class="">+ }<br class=""> AU.addPreserved<GlobalsAAW<wbr class="">rapperPass>();<br class=""> }<br class=""> };<br class="">@@ -653,6 +691,7 @@ struct ADCELegacyPass : public FunctionP<br class=""> char ADCELegacyPass::ID = 0;<br class=""> INITIALIZE_PASS_BEGIN(ADCELega<wbr class="">cyPass, "adce",<br class=""> "Aggress<wbr class="">ive Dead Code Elimination", false, false)<br class="">+INITIALIZE_PASS_DEPENDENCY(Do<wbr class="">minatorTreeWrapperPass)<br class=""> INITIALIZE_PASS_DEPENDENCY(Pos<wbr class="">tDominatorTreeWrapperPass)<br class=""> INITIALIZE_PASS_END(ADCELegacy<wbr class="">Pass, "adce", "Aggressive Dead Code Elimination",<br class=""> false, false)<br class=""><br class="">Added: llvm/trunk/test/Transforms/ADC<wbr class="">E/domtree-DoubleDeletion.ll<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ADCE/domtree-DoubleDeletion.ll?rev=311057&view=auto" target="_blank" class="">http://llvm.org/viewvc/llvm-pr<wbr class="">oject/llvm/trunk/test/Transfor<wbr class="">ms/ADCE/domtree-DoubleDeletion<wbr class="">.ll?rev=311057&view=auto</a><br class="">==============================<wbr class="">==============================<wbr class="">==================<br class="">--- llvm/trunk/test/Transforms/ADC<wbr class="">E/domtree-DoubleDeletion.ll (added)<br class="">+++ llvm/trunk/test/Transforms/ADC<wbr class="">E/domtree-DoubleDeletion.ll Wed Aug 16 18:41:49 2017<br class="">@@ -0,0 +1,39 @@<br class="">+; RUN: opt < %s -gvn -simplifycfg -adce | llvm-dis<br class="">+; RUN: opt < %s -gvn -simplifycfg -adce -verify-dom-info | llvm-dis<br class="">+<br class="">+; This test makes sure that the DominatorTree properly handles<br class="">+; deletion of edges that go to forward-unreachable regions.<br class="">+; In this case, %land.end is already forward unreachable when<br class="">+; the DT gets informed about the deletion of %entry -> %land.end.<br class="">+<br class="">+@a = common global i32 0, align 4<br class="">+<br class="">+define i32 @main() {<br class="">+entry:<br class="">+ %retval = alloca i32, align 4<br class="">+ store i32 0, i32* %retval, align 4<br class="">+ %0 = load i32, i32* @a, align 4<br class="">+ %cmp = icmp ne i32 %0, 1<br class="">+ br i1 %cmp, label %land.rhs, label %land.end4<br class="">+<br class="">+land.rhs: <wbr class=""> ; preds = %entry<br class="">+ %1 = load i32, i32* @a, align 4<br class="">+ %tobool = icmp ne i32 %1, 0<br class="">+ br i1 %tobool, label %land.rhs1, label %land.end<br class="">+<br class="">+land.rhs1: <wbr class=""> ; preds = %land.rhs<br class="">+ br label %land.end<br class="">+<br class="">+land.end: <wbr class=""> ; preds = %land.rhs1, %land.rhs<br class="">+ %2 = phi i1 [ false, %land.rhs ], [ true, %land.rhs1 ]<br class="">+ %land.ext = zext i1 %2 to i32<br class="">+ %conv = trunc i32 %land.ext to i16<br class="">+ %conv2 = sext i16 %conv to i32<br class="">+ %tobool3 = icmp ne i32 %conv2, 0<br class="">+ br label %land.end4<br class="">+<br class="">+land.end4: <wbr class=""> ; preds = %land.end, %entry<br class="">+ %3 = phi i1 [ false, %entry ], [ %tobool3, %land.end ]<br class="">+ %land.ext5 = zext i1 %3 to i32<br class="">+ ret i32 0<br class="">+}<br class=""><br class="">Added: llvm/trunk/test/Transforms/ADC<wbr class="">E/unreachable.ll<br class="">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ADCE/unreachable.ll?rev=311057&view=auto" target="_blank" class="">http://llvm.org/viewvc/llvm-pr<wbr class="">oject/llvm/trunk/test/Transfor<wbr class="">ms/ADCE/unreachable.ll?rev=311<wbr class="">057&view=auto</a><br class="">==============================<wbr class="">==============================<wbr class="">==================<br class="">--- llvm/trunk/test/Transforms/ADC<wbr class="">E/unreachable.ll (added)<br class="">+++ llvm/trunk/test/Transforms/ADC<wbr class="">E/unreachable.ll Wed Aug 16 18:41:49 2017<br class="">@@ -0,0 +1,18 @@<br class="">+; RUN: opt < %s -adce -simplifycfg | llvm-dis<br class="">+; RUN: opt < %s -passes=adce | llvm-dis<br class="">+<br class="">+define i32 @Test(i32 %A, i32 %B) {<br class="">+BB1:<br class="">+ br label %BB4<br class="">+<br class="">+BB2: ; No predecessors!<br class="">+ br label %BB3<br class="">+<br class="">+BB3: ; preds = %BB4, %BB2<br class="">+ %ret = phi i32 [ %X, %BB4 ], [ %B, %BB2 ] ; <i32> [#uses=1]<br class="">+ ret i32 %ret<br class="">+<br class="">+BB4: ; preds = %BB1<br class="">+ %X = phi i32 [ %A, %BB1 ] ; <i32> [#uses=1]<br class="">+ br label %BB3<br class="">+}<br class=""><br class=""><br class="">______________________________<wbr class="">_________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.llvm.org/cgi-bin/<wbr class="">mailman/listinfo/llvm-commits</a><br class=""></div></div></blockquote></div><br class=""></div></div></div></div></blockquote></div><br class=""><br clear="all" class=""><div class=""><br class=""></div></div></div><span class="m_-1183188594685633740HOEnZb"><font color="#888888" class="">-- <br class=""><div class="m_-1183188594685633740m_3621188403423764731gmail_signature" data-smartmail="gmail_signature"><div class="">Jakub Kuderski</div></div>
</font></span></div>
</blockquote></div><br class=""><br clear="all" class=""><div class=""><br class=""></div>-- <br class=""><div class="m_-1183188594685633740gmail_signature" data-smartmail="gmail_signature"><div class="">Jakub Kuderski</div></div>
</div></div>
</div></blockquote></div><br class=""></body></html>