<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Nov 3, 2014 at 5:33 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
> I think DomTree is preserved. In fact, there is a test which fails if<br>
> I *don't* preserve DomTree:<br>
> test/Transforms/InstCombine/assume-loop-align.ll. Hal, you added<br>
> this test recently. This test breaking seems like a bug somewhere.<br>
> Any idea why this test would break like this?<br>
<br>
</span>Sort of; there is a bug in the (legacy) pass manager regarding the availability of non-required analysis passes. There is a note about this at the top of the test, and you'll invalidate the work-around if InstCombine does not preserve DomTree (because DomTree will then not be available to InstCombine, thus making the test fail).<br></blockquote><div><br></div><div>Ah, ok.  Thanks for the explanation. </div><div><br></div><div>Submitted as r221223.</div><div><br></div><div>Mark</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>> The missed optimization I was seeing (dead loop not deleted) was due<br>
> to loop simplify not running. After my patch here's the chain of<br>
> events resulting in the loop being deleted:<br>
> 1) InstCombine runs replacing latch branch predicate with undef ("bra<br>
> undef")<br>
> 2) Loop simplify turns this into always exit at the latch: either<br>
> "bra true" or "bra false". Without my patch loop simplify never<br>
> runs.<br>
> 3) SCEV can now reason about the loop and determine a trip count<br>
> which is trivially one.<br>
> 4) loop deletion deletes loop. (loop-deletion requires a trip count<br>
> to avoid removing infinite loops)<br>
><br>
><br>
> It looks like the only thing not being preserved here is<br>
> LoopSimplify. So I'll add domtree and loopinfo preserved to the<br>
> patch.<br>
<br>
</span>Great!<br>
Thanks,<br>
Hal<br>
<div><div><br>
><br>
> Sounds great. That makes perfect sense. Thanks for the explanation.<br>
><br>
><br>
> -Andy<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> Mark<br>
><br>
><br>
><br>
><br>
> I say, if fixing that is out-of-scope for this change, please file a<br>
> PR for adding an API that updates DomTree and LoopInfo when a branch<br>
> condition changes (to undef).<br>
><br>
> -Andy<br>
><br>
><br>
><br>
> ><br>
> > -Hal<br>
> ><br>
> > ----- Original Message -----<br>
> >> From: "Mark Heffernan" < <a href="mailto:meheff@google.com" target="_blank">meheff@google.com</a> ><br>
> >> To: "Andrew Trick" < <a href="mailto:atrick@apple.com" target="_blank">atrick@apple.com</a> ><br>
> >> Cc: "llvm commits" < <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a> ><br>
> >> Sent: Monday, November 3, 2014 4:36:01 PM<br>
> >> Subject: Re: [PATCH][instcombine] Remove preservesCFG from<br>
> >> instcombine<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> Here's the -debug-pass=Structure output from an O2 build of the<br>
> >> repro<br>
> >> code I posted earlier. The '+' lines are new with the change:<br>
> >><br>
> >><br>
> >><br>
> >> Pass Arguments: -datalayout -notti -basictti -x86tti -no-aa -tbaa<br>
> >> -scoped-noalias -assumption-tracker -targetlibinfo -basicaa<br>
> >> -verify<br>
> >> -simplifycfg -domtree -sroa -early-cse -lower-expect<br>
> >> Data Layout<br>
> >> No target information<br>
> >> Target independent code generator's TTI<br>
> >> X86 Target Transform Info<br>
> >> No Alias Analysis (always returns 'may' alias)<br>
> >> Type-Based Alias Analysis<br>
> >> Scoped NoAlias Alias Analysis<br>
> >> Assumption Tracker<br>
> >> Target Library Information<br>
> >> Basic Alias Analysis (stateless AA impl)<br>
> >> FunctionPass Manager<br>
> >> Module Verifier<br>
> >> Simplify the CFG<br>
> >> Dominator Tree Construction<br>
> >> SROA<br>
> >> Early CSE<br>
> >> Lower 'expect' Intrinsics<br>
> >> -Pass Arguments: -targetlibinfo -datalayout -notti -basictti<br>
> >> -x86tti<br>
> >> -no-aa -tbaa -scoped-noalias -assumption-tracker -basicaa<br>
> >> -verify-di<br>
> >> -ipsccp -globalopt -deadargelim -instcombine -simplifycfg -basiccg<br>
> >> -prune-eh -inline-cost -inline -functionattrs -sroa -domtree<br>
> >> -early-cse -lazy-value-info -jump-threading<br>
> >> -correlated-propagation<br>
> >> -simplifycfg -instcombine -tailcallelim -simplifycfg -reassociate<br>
> >> -domtree -loops -loop-simplify -lcssa -loop-rotate -licm<br>
> >> -loop-unswitch -instcombine -scalar-evolution -lcssa -indvars<br>
> >> -loop-idiom -loop-deletion -function_tti -loop-unroll -memdep<br>
> >> -mldst-motion -domtree -memdep -gvn -memdep -memcpyopt -sccp<br>
> >> -instcombine -lazy-value-info -jump-threading<br>
> >> -correlated-propagation -domtree -memdep -dse -adce -simplifycfg<br>
> >> -instcombine -barrier -domtree -loops -loop-simplify -lcssa<br>
> >> -branch-prob -block-freq -scalar-evolution -loop-vectorize<br>
> >> -instcombine -scalar-evolution -slp-vectorizer -simplifycfg<br>
> >> -instcombine -domtree -loops -loop-simplify -lcssa<br>
> >> -scalar-evolution<br>
> >> -function_tti -loop-unroll -alignment-from-assumptions<br>
> >> -strip-dead-prototypes -globaldce -constmerge -verify -verify-di<br>
> >> -print-module<br>
> >> +Pass Arguments: -targetlibinfo -datalayout -notti -basictti<br>
> >> -x86tti<br>
> >> -no-aa -tbaa -scoped-noalias -assumption-tracker -basicaa<br>
> >> -verify-di<br>
> >> -ipsccp -globalopt -deadargelim -instcombine -simplifycfg -basiccg<br>
> >> -prune-eh -inline-cost -inline -functionattrs -sroa -domtree<br>
> >> -early-cse -lazy-value-info -jump-threading<br>
> >> -correlated-propagation<br>
> >> -simplifycfg -instcombine -tailcallelim -simplifycfg -reassociate<br>
> >> -domtree -loops -loop-simplify -lcssa -loop-rotate -licm<br>
> >> -loop-unswitch -instcombine -domtree -loops -scalar-evolution<br>
> >> -loop-simplify -lcssa -indvars -loop-idiom -loop-deletion<br>
> >> -function_tti -loop-unroll -memdep -mldst-motion -domtree -memdep<br>
> >> -gvn -memdep -memcpyopt -sccp -instcombine -lazy-value-info<br>
> >> -jump-threading -correlated-propagation -domtree -memdep -dse<br>
> >> -adce<br>
> >> -simplifycfg -instcombine -barrier -domtree -loops -loop-simplify<br>
> >> -lcssa -branch-prob -block-freq -scalar-evolution -loop-vectorize<br>
> >> -instcombine -domtree -loops -scalar-evolution -slp-vectorizer<br>
> >> -simplifycfg -instcombine -domtree -loops -loop-simplify -lcssa<br>
> >> -scalar-evolution -function_tti -loop-unroll<br>
> >> -alignment-from-assumptions -strip-dead-prototypes -globaldce<br>
> >> -constmerge -verify -verify-di -print-module<br>
> >> Target Library Information<br>
> >> Data Layout<br>
> >> No target information<br>
> >> Target independent code generator's TTI<br>
> >> X86 Target Transform Info<br>
> >> No Alias Analysis (always returns 'may' alias)<br>
> >> Type-Based Alias Analysis<br>
> >> Scoped NoAlias Alias Analysis<br>
> >> Assumption Tracker<br>
> >> Basic Alias Analysis (stateless AA impl)<br>
> >> ModulePass Manager<br>
> >> Debug Info Verifier<br>
> >> Interprocedural Sparse Conditional Constant Propagation<br>
> >> Global Variable Optimizer<br>
> >> Dead Argument Elimination<br>
> >> FunctionPass Manager<br>
> >> Combine redundant instructions<br>
> >> Simplify the CFG<br>
> >> CallGraph Construction<br>
> >> Call Graph SCC Pass Manager<br>
> >> Remove unused exception handling info<br>
> >> Inline Cost Analysis<br>
> >> Function Integration/Inlining<br>
> >> Deduce function attributes<br>
> >> FunctionPass Manager<br>
> >> SROA<br>
> >> Dominator Tree Construction<br>
> >> Early CSE<br>
> >> Lazy Value Information Analysis<br>
> >> Jump Threading<br>
> >> Value Propagation<br>
> >> Simplify the CFG<br>
> >> Combine redundant instructions<br>
> >> Tail Call Elimination<br>
> >> Simplify the CFG<br>
> >> Reassociate expressions<br>
> >> Dominator Tree Construction<br>
> >> Natural Loop Information<br>
> >> Canonicalize natural loops<br>
> >> Loop-Closed SSA Form Pass<br>
> >> Loop Pass Manager<br>
> >> Rotate Loops<br>
> >> Loop Invariant Code Motion<br>
> >> Unswitch loops<br>
> >> Combine redundant instructions<br>
> >> + Dominator Tree Construction<br>
> >> + Natural Loop Information<br>
> >> Scalar Evolution Analysis<br>
> >> + Canonicalize natural loops<br>
> >> Loop-Closed SSA Form Pass<br>
> >> Loop Pass Manager<br>
> >> Induction Variable Simplification<br>
> >> Recognize loop idioms<br>
> >> Delete dead loops<br>
> >> Function TargetTransformInfo<br>
> >> Loop Pass Manager<br>
> >> Unroll loops<br>
> >> Memory Dependence Analysis<br>
> >> MergedLoadStoreMotion<br>
> >> Dominator Tree Construction<br>
> >> Memory Dependence Analysis<br>
> >> Global Value Numbering<br>
> >> Memory Dependence Analysis<br>
> >> MemCpy Optimization<br>
> >> Sparse Conditional Constant Propagation<br>
> >> Combine redundant instructions<br>
> >> Lazy Value Information Analysis<br>
> >> Jump Threading<br>
> >> Value Propagation<br>
> >> Dominator Tree Construction<br>
> >> Memory Dependence Analysis<br>
> >> Dead Store Elimination<br>
> >> Aggressive Dead Code Elimination<br>
> >> Simplify the CFG<br>
> >> Combine redundant instructions<br>
> >> A No-Op Barrier Pass<br>
> >> FunctionPass Manager<br>
> >> Dominator Tree Construction<br>
> >> Natural Loop Information<br>
> >> Canonicalize natural loops<br>
> >> Loop-Closed SSA Form Pass<br>
> >> Branch Probability Analysis<br>
> >> Block Frequency Analysis<br>
> >> Scalar Evolution Analysis<br>
> >> Loop Vectorization<br>
> >> Combine redundant instructions<br>
> >> + Dominator Tree Construction<br>
> >> + Natural Loop Information<br>
> >> Scalar Evolution Analysis<br>
> >> SLP Vectorizer<br>
> >> Simplify the CFG<br>
> >> Combine redundant instructions<br>
> >> Dominator Tree Construction<br>
> >> Natural Loop Information<br>
> >> Canonicalize natural loops<br>
> >> Loop-Closed SSA Form Pass<br>
> >> Scalar Evolution Analysis<br>
> >> Function TargetTransformInfo<br>
> >> Loop Pass Manager<br>
> >> Unroll loops<br>
> >> Alignment from assumptions<br>
> >> Strip Unused Function Prototypes<br>
> >> Dead Global Elimination<br>
> >> Merge Duplicate Global Constants<br>
> >> FunctionPass Manager<br>
> >> Module Verifier<br>
> >> Debug Info Verifier<br>
> >> Print module to stderr<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> On Mon, Nov 3, 2014 at 1:14 PM, Andrew Trick < <a href="mailto:atrick@apple.com" target="_blank">atrick@apple.com</a> ><br>
> >> wrote:<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> On Nov 3, 2014, at 11:48 AM, Mark Heffernan < <a href="mailto:meheff@google.com" target="_blank">meheff@google.com</a> ><br>
> >> wrote:<br>
> >><br>
> >><br>
> >> Hi Andrew,<br>
> >><br>
> >><br>
> >> (Let me know if someone else is more appropriate to review this).<br>
> >> This patch removes setPreservesCFG() from the instcombine pass.<br>
> >> The<br>
> >> reason is that InstCombine can modify terminator instructions<br>
> >> (branches) specifically the predicate. Instcombine clears out dead<br>
> >> blocks and replaces all the dead instruction uses with undef:<br>
> >><br>
> >><br>
> >> <a href="http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l02792" target="_blank">http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l02792</a><br>
> >><br>
> >><br>
> >><br>
> >> These new undefs can be predicates of branches, and from the<br>
> >> comment<br>
> >> of setPreservesCFG():<br>
> >><br>
> >><br>
> >><br>
> >> // setPreservesCFG - This function should be called to by the<br>
> >> pass,<br>
> >> iff they do<br>
> >> // not:<br>
> >> //<br>
> >> // 1. Add or remove basic blocks from the function<br>
> >> // 2. Modify terminator instructions in any way.<br>
> >><br>
> >><br>
> >> Clearly instcombine is modifying terminator instructions so should<br>
> >> not be calling setPreservesCFG(). The specific issue I ran into<br>
> >> was<br>
> >> a dead loop not being deleted because of stale analysis data.<br>
> >> Repro:<br>
> >><br>
> >><br>
> >><br>
> >> void foo(int *a, int start_x, int end_x, int start_y, int end_y,<br>
> >> int<br>
> >> k) {<br>
> >> for (int y = start_y; y < end_y; y++) {<br>
> >> for (int x = start_x; x < end_x; x++) {<br>
> >> for (int i = 0; i < 1000; i++) {<br>
> >> a[x + y + k*i] = 0;<br>
> >> }<br>
> >> }<br>
> >> }<br>
> >> }<br>
> >><br>
> >><br>
> >> # Dead loop left after loop deletion:<br>
> >> clang -S -emit-llvm foo.c -o - | opt -O1 -loop-rotate<br>
> >> -loop-unswitch<br>
> >> -instcombine -loop-deletion -S > bad.ll<br>
> >><br>
> >><br>
> >><br>
> >> # Dead loop removed by splitting out loop-deletion into separate<br>
> >> opt<br>
> >> invocation:<br>
> >> clang -S -emit-llvm foo.c -o - | opt -O1 -loop-rotate<br>
> >> -loop-unswitch<br>
> >> -instcombine | opt -loop-deletion -S > good.ll<br>
> >><br>
> >><br>
> >><br>
> >> This patch fixes this discrepancy.<br>
> >><br>
> >> This seems totally reasonable given the current (lack of)<br>
> >> invalidation mechanisms.<br>
> >><br>
> >><br>
> >> For the record, can you report the difference between<br>
> >> -debug-pass=Structure before and after output?<br>
> >><br>
> >><br>
> >> -Andy<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> Mark <no_cfg_preserve_instcombine.patch><br>
> >><br>
> >><br>
> >> _______________________________________________<br>
> >> llvm-commits mailing list<br>
> >> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> >><br>
> ><br>
> > --<br>
> > Hal Finkel<br>
> > Assistant Computational Scientist<br>
> > Leadership Computing Facility<br>
> > Argonne National Laboratory<br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div></div>