<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Nov 3, 2014 at 3:44 PM, Andrew Trick <span dir="ltr"><<a href="mailto:atrick@apple.com" target="_blank">atrick@apple.com</a>></span> wrote:<br><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 class=""><br>
> On Nov 3, 2014, at 2:49 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br>
><br>
> Can we still have InstCombine preserve the DomTree?<br>
<br>
</span>InstCombine *really* should preserve DomTree and LoopInfo. But one of those may be causing the missed optimization. Maybe DominatorTree::isReachableFromEntry needs to be updated? That could in turn affect LoopInfo or LCSSA.<br></blockquote><div><br></div><div>I think DomTree is preserved.  In fact, there is a test which fails if I *don't* preserve DomTree: test/Transforms/InstCombine/assume-loop-align.ll.  Hal, you added this test recently.  This test breaking seems like a bug somewhere.  Any idea why this test would break like this?</div><div><br></div><div>The missed optimization I was seeing (dead loop not deleted) was due to loop simplify not running.  After my patch here's the chain of events resulting in the loop being deleted:</div><div>1) InstCombine runs replacing latch branch predicate with undef ("bra undef")</div><div>2) Loop simplify turns this into always exit at the latch: either "bra true" or "bra false".  Without my patch loop simplify never runs.</div><div>3) SCEV can now reason about the loop and determine a trip count which is trivially one.</div><div>4) loop deletion deletes loop.  (loop-deletion requires a trip count to avoid removing infinite loops)</div><div><br></div><div>It looks like the only thing not being preserved here is LoopSimplify.  So I'll add domtree and loopinfo preserved to the patch.</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">
<br>
I say, if fixing that is out-of-scope for this change, please file a PR for adding an API that updates DomTree and LoopInfo when a branch condition changes (to undef).<br>
<br>
-Andy<br>
<div class=""><div class="h5"><br>
><br>
> -Hal<br>
><br>
> ----- Original Message -----<br>
>> From: "Mark Heffernan" <<a href="mailto:meheff@google.com">meheff@google.com</a>><br>
>> To: "Andrew Trick" <<a href="mailto:atrick@apple.com">atrick@apple.com</a>><br>
>> Cc: "llvm commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
>> Sent: Monday, November 3, 2014 4:36:01 PM<br>
>> Subject: Re: [PATCH][instcombine] Remove preservesCFG from instcombine<br>
>><br>
>><br>
>><br>
>><br>
>> Here's the -debug-pass=Structure output from an O2 build of the 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 -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 -x86tti<br>
>> -no-aa -tbaa -scoped-noalias -assumption-tracker -basicaa -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 -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 -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 -x86tti<br>
>> -no-aa -tbaa -scoped-noalias -assumption-tracker -basicaa -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 -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 -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">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">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. 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 comment<br>
>> of setPreservesCFG():<br>
>><br>
>><br>
>><br>
>> // setPreservesCFG - This function should be called to by the 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 was<br>
>> a dead loop not being deleted because of stale analysis data. Repro:<br>
>><br>
>><br>
>><br>
>> void foo(int *a, int start_x, int end_x, int start_y, int end_y, 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 -loop-unswitch<br>
>> -instcombine -loop-deletion -S > bad.ll<br>
>><br>
>><br>
>><br>
>> # Dead loop removed by splitting out loop-deletion into separate opt<br>
>> invocation:<br>
>> clang -S -emit-llvm foo.c -o - | opt -O1 -loop-rotate -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">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>
</div></div></blockquote></div><br></div></div>