<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 27, 2015 at 1:34 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Refactoring should be easy.<br><br>The whole set of blocks can be replaced with:<br><div><div>  // Scan the function bodies for explicit loads or stores.</div><div>    for (unsigned i = 0, e = SCC.size(); i != e && FunctionEffect != ModRef;++i)</div><div>      for (inst_iterator II = inst_begin(SCC[i]->getFunction()),</div><div>             E = inst_end(SCC[i]->getFunction());</div><div>           II != E && FunctionEffect != ModRef; ++II)</div><div>{</div><div>FunctionEffect |= getModRefInfo(&*II);</div><div>}</div><div><br></div></div><div><br>This should fix your bug too (your code misses FenceInst, VAArgInst, and a few others) ;)</div><div><br></div><div>The only one i'm not 100% positive this covers is isAllocationFn and isFreeFn, but we should get the right answer here anyway if i'm following call chains right.</div></div></blockquote><div><br></div><div><br></div><div>Any idea about the best way to test this? We could extend the testing in the original patch to cover other instructions, but I feel like having the list of instructions in the test is as fragile as open-coding the list of instructions in the code.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5"><div><br></div><div class="gmail_quote">On Mon, Apr 27, 2015 at 1:16 PM Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Apr 25, 2015 at 2:21 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><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"><div><div>----- Original Message -----<br>
> From: "Sean Silva" <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>><br>
> To: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> Cc: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>>, "Nick Lewycky" <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>>, "Philip Reames" <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>><br>
> Sent: Friday, April 24, 2015 10:56:58 PM<br>
> Subject: [PATCH] [globalsmodref-aa] Atomics *DO* touch memory...<br>
><br>
><br>
> GlobalsModRef was giving a spurious alias analysis result because it<br>
> failed to consider that atomics access memory. The attached patch<br>
> fixes that (PR23345).<br>
><br>
><br>
> I reduced this from a "compare and swap" loop that looked funny in<br>
> the machine code: the "compare and swap" had in fact been hoisted<br>
> out of the loop. I only ran into this when I tried `-flto -O2<br>
> -fno-inline`.<br>
><br>
><br>
> The structure of the test is pretty closely copied from one of the<br>
> others in the directory. Any better ideas for testing this (or ways<br>
> to make the testing more thorough) would be appreciated.<br>
><br>
><br>
> If somebody can doublecheck that these are the only missing cases,<br>
> that would be appreciated. This loop been touched since 2012 and I'm<br>
> relatively clueless about AA. Hopefully this patch also suggests a<br>
> couple other places to audit for similar bugs to those with a better<br>
> "big picture" of the IR analysis and transforms than myself.<br>
<br>
</div></div>Hi Sean,<br>
<br>
The list is also missing VAArgInst, although maybe that's on purpose (although, in that case, a comment would be nice).<br>
<br>
While we're looking at this, other things I don't understand about this loop are 1) Why is is special casing alloc/free functions and 2) why is it treating all volatile accesses as read/write?<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I know at least for volatile accesses it is because it might be a hardware register that has some arbitrary effect on other memory. A simple example is ARM's "bit-banding".</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div> </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>
It might be nice to refactor this code to call<br>
  ModRefResult getModRefInfo(const Instruction *I)<br>
from AliasAnalysis. That way we could reduce the number of places that replicate this kind of logic.<br></blockquote><div><br></div><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Hal, others: I'm really not an expert in AA or even really have that much experience at the IR level. Would one of you be willing to comandeer this patch from me? I'm afraid that I'm mostly going to be slowing things down here.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>-- Sean Silva</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </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>
 -Hal<br>
<br>
><br>
><br>
> -- Sean Silva<br>
<span><font color="#888888"><br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div></div></div></blockquote></div></div></div></div>
</blockquote></div><br></div></div>