<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 26, 2016 at 2:30 PM,  <span dir="ltr"><<a href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 2016-10-26 15:39, Daniel Berlin via llvm-commits wrote:<br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
On Wed, Oct 26, 2016 at 12:28 PM, Chad Rosier<br>
<<a href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>> wrote:<br>
<br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
mcrosier added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D25969#580042" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2596<wbr>9#580042</a> [1], @mcrosier wrote:<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In <a href="https://reviews.llvm.org/D25969#579967" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2596<wbr>9#579967</a> [2], @nemanjai wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This caused a failure on all PPC machines I have access to:<br>
 AddressSanitizer-powerpc64le-<wbr>linux ::<br>
</blockquote></blockquote>
TestCases/use-after-scope-loop<wbr>-bug.cc<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Looking now..<br>
</blockquote>
<br>
I believe I've figured out the underlying problem.  In short, LICM<br>
is relying on lifetime markers to prevent loads/stores from being<br>
moved out of a particular scope.<br>
</span></blockquote><span class="">
<br>
Can you file more details on a bug somewhere? This seems pretty<br>
broken, but also, without your patch:<br>
<br>
dannyb@dannyb-macbookpro3 [12:32:05]<br>
[~/sources/llvm-clean/lib/Tran<wbr>sforms/Scalar] [mssa-batch-renamer-2]<br>
-> % grep -i lifetime LICM.cpp<br>
<br>
</span></blockquote>
<br>
Right.  LICM doesn't explicitly check for the lifetime intrinsics.  Rather, the lifetime intrinsics cause the AliasSetTracker to collapse sets, which indirectly changes the behavior of LICM with uses ASTs.</blockquote><div><br></div><div>Right.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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">
>From the broken test case:<br>
int main() {<br>
<br>
// Variable goes in and out of scope.<br>
for (int i = 0; i < 3; ++i) {<br>
int x[3] = {i, i, i};<br>
p = x + i;<br>
}<br>
return *p;  // BOOM<br>
<br>
}<br>
<br>
LICM will sink the stores to x[3] after the loop, which is obviously<br>
incorrect.<br>
</blockquote>
<br>
I don't see why?<br>
</blockquote>
<br></span>
I assumed creating stores to the array after the lifetime of the array had ended was incorrect, but I guess that's not really true given the alloc does not live inside the loop.</blockquote><div><br></div><div>Right, this is really a test of "does this get translated in a way that ASAN detects it", precisely because it's legal to do the sinking "in llvm ir" but "not in c"</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">  <br></span></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I've reverted the change in r285227.  Sorry for the breakage.<br>
</blockquote>
<br>
You should be able to fix this simply by either<br>
1. dropping the lifetime markers (non-ideal) of things you sink.<br>
or<br>
2.  extending the lifetime markers appropriately.<br>
<br>
The algorithm for that is that when you eliminate or sink things past<br>
them, you move the end to wherever the lowest point you sunk to is.<br>
You move starts to a block that post-dominates both the new end and<br>
the old end.<br>
</blockquote>
<br></span>
Honestly, this is something I saw in passing and I'm not sure I want to tackle this problem at th e moment.  However, I'll make sure to create a details PR in the morning.  Perhaps, I'll have some free time in the future.<br></blockquote><div><br></div><div>SGTM.</div><div>If there's a PR, and someone discovers it improves performance, i'm sure it'll be gotten to eventually :)</div><div>Here, the PR now describes what to do and how to fix it, which is better than most of our PR's :)<br><br></div><div><br></div><div><br></div></div></div></div>