<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Nov 3, 2014, at 4:24 PM, Mark Heffernan <<a href="mailto:meheff@google.com" class="">meheff@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_extra"><div class="gmail_quote">On Mon, Nov 3, 2014 at 3:44 PM, Andrew Trick<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:atrick@apple.com" target="_blank" class="">atrick@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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 class="">> On Nov 3, 2014, at 2:49 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov" class="">hfinkel@anl.gov</a>> wrote:<br class="">><br class="">> Can we still have InstCombine preserve the DomTree?<br class=""><br class=""></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 class=""></blockquote><div class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class="">1) InstCombine runs replacing latch branch predicate with undef ("bra undef")</div><div class="">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 class="">3) SCEV can now reason about the loop and determine a trip count which is trivially one.</div><div class="">4) loop deletion deletes loop. (loop-deletion requires a trip count to avoid removing infinite loops)</div><div class=""><br class=""></div><div class="">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></div></div></div></blockquote><div><br class=""></div>Sounds great. That makes perfect sense. Thanks for the explanation.</div><div><br class=""></div><div>-Andy</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class="">Mark</div><div class=""><br class=""></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 class="">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 class=""><br class="">-Andy<br class=""><div class=""><div class="h5"><br class="">><br class="">> -Hal<br class="">><br class="">> ----- Original Message -----<br class="">>> From: "Mark Heffernan" <<a href="mailto:meheff@google.com" class="">meheff@google.com</a>><br class="">>> To: "Andrew Trick" <<a href="mailto:atrick@apple.com" class="">atrick@apple.com</a>><br class="">>> Cc: "llvm commits" <<a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a>><br class="">>> Sent: Monday, November 3, 2014 4:36:01 PM<br class="">>> Subject: Re: [PATCH][instcombine] Remove preservesCFG from instcombine<br class="">>><br class="">>><br class="">>><br class="">>><br class="">>> Here's the -debug-pass=Structure output from an O2 build of the repro<br class="">>> code I posted earlier. The '+' lines are new with the change:<br class="">>><br class="">>><br class="">>><br class="">>> Pass Arguments: -datalayout -notti -basictti -x86tti -no-aa -tbaa<br class="">>> -scoped-noalias -assumption-tracker -targetlibinfo -basicaa -verify<br class="">>> -simplifycfg -domtree -sroa -early-cse -lower-expect<br class="">>> Data Layout<br class="">>> No target information<br class="">>> Target independent code generator's TTI<br class="">>> X86 Target Transform Info<br class="">>> No Alias Analysis (always returns 'may' alias)<br class="">>> Type-Based Alias Analysis<br class="">>> Scoped NoAlias Alias Analysis<br class="">>> Assumption Tracker<br class="">>> Target Library Information<br class="">>> Basic Alias Analysis (stateless AA impl)<br class="">>> FunctionPass Manager<br class="">>> Module Verifier<br class="">>> Simplify the CFG<br class="">>> Dominator Tree Construction<br class="">>> SROA<br class="">>> Early CSE<br class="">>> Lower 'expect' Intrinsics<br class="">>> -Pass Arguments: -targetlibinfo -datalayout -notti -basictti -x86tti<br class="">>> -no-aa -tbaa -scoped-noalias -assumption-tracker -basicaa -verify-di<br class="">>> -ipsccp -globalopt -deadargelim -instcombine -simplifycfg -basiccg<br class="">>> -prune-eh -inline-cost -inline -functionattrs -sroa -domtree<br class="">>> -early-cse -lazy-value-info -jump-threading -correlated-propagation<br class="">>> -simplifycfg -instcombine -tailcallelim -simplifycfg -reassociate<br class="">>> -domtree -loops -loop-simplify -lcssa -loop-rotate -licm<br class="">>> -loop-unswitch -instcombine -scalar-evolution -lcssa -indvars<br class="">>> -loop-idiom -loop-deletion -function_tti -loop-unroll -memdep<br class="">>> -mldst-motion -domtree -memdep -gvn -memdep -memcpyopt -sccp<br class="">>> -instcombine -lazy-value-info -jump-threading<br class="">>> -correlated-propagation -domtree -memdep -dse -adce -simplifycfg<br class="">>> -instcombine -barrier -domtree -loops -loop-simplify -lcssa<br class="">>> -branch-prob -block-freq -scalar-evolution -loop-vectorize<br class="">>> -instcombine -scalar-evolution -slp-vectorizer -simplifycfg<br class="">>> -instcombine -domtree -loops -loop-simplify -lcssa -scalar-evolution<br class="">>> -function_tti -loop-unroll -alignment-from-assumptions<br class="">>> -strip-dead-prototypes -globaldce -constmerge -verify -verify-di<br class="">>> -print-module<br class="">>> +Pass Arguments: -targetlibinfo -datalayout -notti -basictti -x86tti<br class="">>> -no-aa -tbaa -scoped-noalias -assumption-tracker -basicaa -verify-di<br class="">>> -ipsccp -globalopt -deadargelim -instcombine -simplifycfg -basiccg<br class="">>> -prune-eh -inline-cost -inline -functionattrs -sroa -domtree<br class="">>> -early-cse -lazy-value-info -jump-threading -correlated-propagation<br class="">>> -simplifycfg -instcombine -tailcallelim -simplifycfg -reassociate<br class="">>> -domtree -loops -loop-simplify -lcssa -loop-rotate -licm<br class="">>> -loop-unswitch -instcombine -domtree -loops -scalar-evolution<br class="">>> -loop-simplify -lcssa -indvars -loop-idiom -loop-deletion<br class="">>> -function_tti -loop-unroll -memdep -mldst-motion -domtree -memdep<br class="">>> -gvn -memdep -memcpyopt -sccp -instcombine -lazy-value-info<br class="">>> -jump-threading -correlated-propagation -domtree -memdep -dse -adce<br class="">>> -simplifycfg -instcombine -barrier -domtree -loops -loop-simplify<br class="">>> -lcssa -branch-prob -block-freq -scalar-evolution -loop-vectorize<br class="">>> -instcombine -domtree -loops -scalar-evolution -slp-vectorizer<br class="">>> -simplifycfg -instcombine -domtree -loops -loop-simplify -lcssa<br class="">>> -scalar-evolution -function_tti -loop-unroll<br class="">>> -alignment-from-assumptions -strip-dead-prototypes -globaldce<br class="">>> -constmerge -verify -verify-di -print-module<br class="">>> Target Library Information<br class="">>> Data Layout<br class="">>> No target information<br class="">>> Target independent code generator's TTI<br class="">>> X86 Target Transform Info<br class="">>> No Alias Analysis (always returns 'may' alias)<br class="">>> Type-Based Alias Analysis<br class="">>> Scoped NoAlias Alias Analysis<br class="">>> Assumption Tracker<br class="">>> Basic Alias Analysis (stateless AA impl)<br class="">>> ModulePass Manager<br class="">>> Debug Info Verifier<br class="">>> Interprocedural Sparse Conditional Constant Propagation<br class="">>> Global Variable Optimizer<br class="">>> Dead Argument Elimination<br class="">>> FunctionPass Manager<br class="">>> Combine redundant instructions<br class="">>> Simplify the CFG<br class="">>> CallGraph Construction<br class="">>> Call Graph SCC Pass Manager<br class="">>> Remove unused exception handling info<br class="">>> Inline Cost Analysis<br class="">>> Function Integration/Inlining<br class="">>> Deduce function attributes<br class="">>> FunctionPass Manager<br class="">>> SROA<br class="">>> Dominator Tree Construction<br class="">>> Early CSE<br class="">>> Lazy Value Information Analysis<br class="">>> Jump Threading<br class="">>> Value Propagation<br class="">>> Simplify the CFG<br class="">>> Combine redundant instructions<br class="">>> Tail Call Elimination<br class="">>> Simplify the CFG<br class="">>> Reassociate expressions<br class="">>> Dominator Tree Construction<br class="">>> Natural Loop Information<br class="">>> Canonicalize natural loops<br class="">>> Loop-Closed SSA Form Pass<br class="">>> Loop Pass Manager<br class="">>> Rotate Loops<br class="">>> Loop Invariant Code Motion<br class="">>> Unswitch loops<br class="">>> Combine redundant instructions<br class="">>> + Dominator Tree Construction<br class="">>> + Natural Loop Information<br class="">>> Scalar Evolution Analysis<br class="">>> + Canonicalize natural loops<br class="">>> Loop-Closed SSA Form Pass<br class="">>> Loop Pass Manager<br class="">>> Induction Variable Simplification<br class="">>> Recognize loop idioms<br class="">>> Delete dead loops<br class="">>> Function TargetTransformInfo<br class="">>> Loop Pass Manager<br class="">>> Unroll loops<br class="">>> Memory Dependence Analysis<br class="">>> MergedLoadStoreMotion<br class="">>> Dominator Tree Construction<br class="">>> Memory Dependence Analysis<br class="">>> Global Value Numbering<br class="">>> Memory Dependence Analysis<br class="">>> MemCpy Optimization<br class="">>> Sparse Conditional Constant Propagation<br class="">>> Combine redundant instructions<br class="">>> Lazy Value Information Analysis<br class="">>> Jump Threading<br class="">>> Value Propagation<br class="">>> Dominator Tree Construction<br class="">>> Memory Dependence Analysis<br class="">>> Dead Store Elimination<br class="">>> Aggressive Dead Code Elimination<br class="">>> Simplify the CFG<br class="">>> Combine redundant instructions<br class="">>> A No-Op Barrier Pass<br class="">>> FunctionPass Manager<br class="">>> Dominator Tree Construction<br class="">>> Natural Loop Information<br class="">>> Canonicalize natural loops<br class="">>> Loop-Closed SSA Form Pass<br class="">>> Branch Probability Analysis<br class="">>> Block Frequency Analysis<br class="">>> Scalar Evolution Analysis<br class="">>> Loop Vectorization<br class="">>> Combine redundant instructions<br class="">>> + Dominator Tree Construction<br class="">>> + Natural Loop Information<br class="">>> Scalar Evolution Analysis<br class="">>> SLP Vectorizer<br class="">>> Simplify the CFG<br class="">>> Combine redundant instructions<br class="">>> Dominator Tree Construction<br class="">>> Natural Loop Information<br class="">>> Canonicalize natural loops<br class="">>> Loop-Closed SSA Form Pass<br class="">>> Scalar Evolution Analysis<br class="">>> Function TargetTransformInfo<br class="">>> Loop Pass Manager<br class="">>> Unroll loops<br class="">>> Alignment from assumptions<br class="">>> Strip Unused Function Prototypes<br class="">>> Dead Global Elimination<br class="">>> Merge Duplicate Global Constants<br class="">>> FunctionPass Manager<br class="">>> Module Verifier<br class="">>> Debug Info Verifier<br class="">>> Print module to stderr<br class="">>><br class="">>><br class="">>><br class="">>><br class="">>> On Mon, Nov 3, 2014 at 1:14 PM, Andrew Trick <<span class="Apple-converted-space"> </span><a href="mailto:atrick@apple.com" class="">atrick@apple.com</a><span class="Apple-converted-space"> </span>><br class="">>> wrote:<br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>> On Nov 3, 2014, at 11:48 AM, Mark Heffernan <<span class="Apple-converted-space"> </span><a href="mailto:meheff@google.com" class="">meheff@google.com</a><span class="Apple-converted-space"> </span>><br class="">>> wrote:<br class="">>><br class="">>><br class="">>> Hi Andrew,<br class="">>><br class="">>><br class="">>> (Let me know if someone else is more appropriate to review this).<br class="">>> This patch removes setPreservesCFG() from the instcombine pass. The<br class="">>> reason is that InstCombine can modify terminator instructions<br class="">>> (branches) specifically the predicate. Instcombine clears out dead<br class="">>> blocks and replaces all the dead instruction uses with undef:<br class="">>><br class="">>><br class="">>><span class="Apple-converted-space"> </span><a href="http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l02792" target="_blank" class="">http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l02792</a><br class="">>><br class="">>><br class="">>><br class="">>> These new undefs can be predicates of branches, and from the comment<br class="">>> of setPreservesCFG():<br class="">>><br class="">>><br class="">>><br class="">>> // setPreservesCFG - This function should be called to by the pass,<br class="">>> iff they do<br class="">>> // not:<br class="">>> //<br class="">>> // 1. Add or remove basic blocks from the function<br class="">>> // 2. Modify terminator instructions in any way.<br class="">>><br class="">>><br class="">>> Clearly instcombine is modifying terminator instructions so should<br class="">>> not be calling setPreservesCFG(). The specific issue I ran into was<br class="">>> a dead loop not being deleted because of stale analysis data. Repro:<br class="">>><br class="">>><br class="">>><br class="">>> void foo(int *a, int start_x, int end_x, int start_y, int end_y, int<br class="">>> k) {<br class="">>> for (int y = start_y; y < end_y; y++) {<br class="">>> for (int x = start_x; x < end_x; x++) {<br class="">>> for (int i = 0; i < 1000; i++) {<br class="">>> a[x + y + k*i] = 0;<br class="">>> }<br class="">>> }<br class="">>> }<br class="">>> }<br class="">>><br class="">>><br class="">>> # Dead loop left after loop deletion:<br class="">>> clang -S -emit-llvm foo.c -o - | opt -O1 -loop-rotate -loop-unswitch<br class="">>> -instcombine -loop-deletion -S > bad.ll<br class="">>><br class="">>><br class="">>><br class="">>> # Dead loop removed by splitting out loop-deletion into separate opt<br class="">>> invocation:<br class="">>> clang -S -emit-llvm foo.c -o - | opt -O1 -loop-rotate -loop-unswitch<br class="">>> -instcombine | opt -loop-deletion -S > good.ll<br class="">>><br class="">>><br class="">>><br class="">>> This patch fixes this discrepancy.<br class="">>><br class="">>> This seems totally reasonable given the current (lack of)<br class="">>> invalidation mechanisms.<br class="">>><br class="">>><br class="">>> For the record, can you report the difference between<br class="">>> -debug-pass=Structure before and after output?<br class="">>><br class="">>><br class="">>> -Andy<br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>><br class="">>> Mark <no_cfg_preserve_instcombine.patch><br class="">>><br class="">>><br class="">>> _______________________________________________<br class="">>> llvm-commits mailing list<br class="">>><span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class="">>><span class="Apple-converted-space"> </span><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class="">>><br class="">><br class="">> --<br class="">> Hal Finkel<br class="">> Assistant Computational Scientist<br class="">> Leadership Computing Facility<br class="">> Argonne National Laboratory</div></div></blockquote></div></div></div></div></blockquote></div><br class=""></body></html>