<div dir="ltr">LGTM.<div>Thank you</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 9, 2016 at 9:59 AM, Anna Thomas <span dir="ltr"><<a href="mailto:anna@azul.com" target="_blank">anna@azul.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">anna marked 2 inline comments as done.<br>
<br>
================<br>
Comment at: lib/Analysis/<wbr>BasicAliasAnalysis.cpp:785<br>
@@ +784,3 @@<br>
+  // Like assumes, invariant.start intrinsics are also marked as arbitrarily<br>
+  // writing so that proper control dependencies are maintained but they never<br>
+  // mod any particular memory location visible to the IR.<br>
----------------<br>
anna wrote:<br>
> dberlin wrote:<br>
> > Errr, this is now wrong?<br>
> ><br>
> > Maybe you mean "were both also marked"?<br>
> I'm not sure if the comment is wrong. The comment intended to convey: "In other code such as MDA, instruction->MayWriteToMemory, etc, assumes and invariant.start are marked as read/write, even though they do not modify memory" However, while assume is marked as NoModRef above, we need to mark invariant.start as reading memory.<br>
><br>
> It actually follows from the above 2 intrinsic comments "assume and experimental_guard" :)<br>
> Should I update the comment?<br>
><br>
I've updated the comment to:<br>
Like assumes, invariant.start intrinsics were also marked as arbitrarily<br>
writing so that proper control dependencies are maintained but they never<br>
mod any particular memory location visible to the IR.<br>
*Unlike* assumes (which are now modeled as NoModRef), invariant.start intrinsic is now modeled as reading memory. This prevents hoisting the invariant.start intrinsic over stores.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D23214" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D23214</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>