<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 1 September 2014 03:22, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ahem, this probably shouldn't have gone in quite yet. ;] It has some bad bugs in it.</blockquote><div><br></div><div>

Doh, sorry. Before commit I spent a while thinking about where lifetime intrinsics could be placed such that ignoring them would make the transformation invalid (ie., you start relying on a lifetime.end'd value where previously you didn't). I didn't actually review the implementation.</div>

<div><br></div><div>Thank you for taking care of this.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> In the future, I would suggest always including a negative test (or ensuring there is an existing negative test) to make sure the transformation logic you intended to have is in fact there.<br>


<br>
See below for the details. I've gone ahead and fixed this in ToT to unbreak things.<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:682-685<br>
@@ -681,2 +681,6 @@<br>
<div class="">         return false;<br>
+    } else if (const IntrinsicInst *IT = dyn_cast<IntrinsicInst>(U)) {<br>
+      if (IT->getIntrinsicID() != Intrinsic::lifetime_start &&<br>
+          IT->getIntrinsicID() != Intrinsic::lifetime_end)<br>
+        continue;<br>
     } else if (U != C && U != cpy) {<br>
</div>----------------<br>
This is wrong in several ways.<br>
<br>
First off, it is identifying *non* lifetime intrinsics here, not lifetime intrinsics. But *non* lifetime intrinsics definitely shouldn't be "continue". I think the logic of the if is just reversed.<br>
<br>
The other problem is that falling through the if... actually just does exactly the same thing as the body of the if -- it continues. This is equivalent to just continuing for *all* intrinsics, lifetime or otherwise. That in turn can miscompile code if the intrinsics are actually using the pointer and are not either C or cpy as checked below.<br>


<br>
<br>
I've substantially rewritten the patch in r216871 to address both of these points. Please review it to make sure it matches what you expected.<br>
<br>
<a href="http://reviews.llvm.org/D5020" target="_blank">http://reviews.llvm.org/D5020</a><br>
<br>
<br>
</blockquote></div><br></div></div>