<div dir="ltr">It seems to me that LoopSimplify is the problem here. Should LoopSimplifyPass be marked as IsCFGOnlyPass? It clearly modifies the CFG. From the "writing an llvm pass" manual: "if a pass walks CFG without modifying it then the third argument [isCFGOnly] is set to true", however, I don't know how definitive that is and the logic of that statement doesn't exactly imply that a CFG-modifying pass can't be marked isCFGOnly. If LoopSimplifyPass is changed to !isCFGOnly, then preservesCFG can be added back to instcombine (at least for the purposes of the bug I ran into).<div><br></div><div>Mark</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 4, 2014 at 11:23 AM, Nick Lewycky <span dir="ltr"><<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Hal Finkel wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
----- Original Message -----<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
From: "Nick Lewycky"<<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>><br>
To: "Mark Heffernan"<<a href="mailto:meheff@google.com" target="_blank">meheff@google.com</a>><br>
Cc: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
Sent: Tuesday, November 4, 2014 2:28:20 AM<br>
Subject: Re: [PATCH][instcombine] Remove preservesCFG from instcombine<br>
<br>
Mark Heffernan wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Andrew,<br>
<br>
(Let me know if someone else is more appropriate to review this).<br>
This<br>
patch removes setPreservesCFG() from the instcombine pass.<br>
</blockquote>
<br>
NAK. Please don't commit this.<br>
</blockquote>
<br>
Already happened (r221223) [but, via the wonders of version control...]<br>
</blockquote>
<br></span>
Fair enough. I would like it changed, either fixed forwards or backed out, but this is not urgent and we can decide what the right thing to do is first.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The reason<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
is that InstCombine can modify terminator instructions (branches)<br>
specifically the predicate.<br>
</blockquote>
<br>
This is the thing where it removes an '%newcond = xor %cond, true'<br>
and<br>
switches 'br %newcond, label %A, label %B' with 'br %cond, label %B,<br>
label %A'?<br>
<br>
If it's actually causing you problems, please just move that<br>
transform<br>
to simplifycfg instead of removing preserves-cfg from instcombine.<br>
</blockquote>
<br>
It seems like the problem is more general than that:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The reason is that InstCombine can modify terminator instructions (branches) specifically the predicate. Instcombine clears out dead blocks and replaces all the dead instruction uses with undef:<br>
<br>
<a href="http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l02792" target="_blank">http://llvm.org/docs/doxygen/<u></u>html/InstructionCombining_<u></u>8cpp_source.html#l02792</a><br>
<br>
These new undefs can be predicates of branches, and from the comment of setPreservesCFG():<br>
</blockquote></blockquote>
<br>
So perhaps any optimization that produces an undef, where that undef could possibly propagate to a branch condition, could under iteration cause this problem. And the code above there says:<br>
<br>
// Do a quick scan over the function. If we find any blocks that are<br>
// unreachable, remove any instructions inside of them. This prevents<br>
// the instcombine code from having to deal with some bad special cases.<br>
<br>
So this may actually be trickier to solve otherwise than it at-first appears.<br>
</blockquote>
<br></span>
Clearly a CFG preserving pass may change the predicate to a branch. An optimization which determines that i1 %x is always true may run %x->RAUW(i1 true), it would be terrible if preserving the CFG would prevent that.<br>
<br>
This implies that CFG driven analyses need to be prepared to have the branch predicates (and switch predicates, and invoke arguments) change under them, so long as the change doesn't have a semantic difference. If an SSA value gets replaced with undef, that's because it always was undef, it just wasn't easy to see previously. So I think the place to look is at LoopInfo and its handling of undef. Currently we treat undef as "can branch to either side, not sure which". What is LoopInfo doing?<span class="HOEnZb"><font color="#888888"><br>
<br>
Nick</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
-Hal<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Nick<br>
<br>
Instcombine clears out dead blocks and<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
replaces all the dead instruction uses with undef:<br>
<br>
<a href="http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l02792" target="_blank">http://llvm.org/docs/doxygen/<u></u>html/InstructionCombining_<u></u>8cpp_source.html#l02792</a><br>
<br>
These new undefs can be predicates of branches, and from the<br>
comment<br>
of setPreservesCFG():<br>
<br>
// setPreservesCFG - This function should be called to by the pass,<br>
iff<br>
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>
Clearly instcombine is modifying terminator instructions so should<br>
not<br>
be calling setPreservesCFG(). The specific issue I ran into was a<br>
dead<br>
loop not being deleted because of stale analysis data. Repro:<br>
<br>
void foo(int *a, int start_x, int end_x, int start_y, int end_y,<br>
int 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>
# 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>
# 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>
This patch fixes this discrepancy.<br>
<br>
Mark<br>
<br>
<br>
______________________________<u></u>_________________<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/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
______________________________<u></u>_________________<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/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</div></div></blockquote></div><br></div>