<div dir="ltr">Thanks! I think this fixed PR23345; I didn't know about <span style="font-size:13px">I.mayWriteToMemory()</span><div><br></div><div>-- Sean Silva</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 15, 2015 at 1:53 AM, 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">Author: chandlerc<br>
Date: Wed Jul 15 03:53:29 2015<br>
New Revision: 242281<br>
<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D242281-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=v_2U7OQd83CL4gchkkSZVU3fb0tMVRRG0j7MxIm2l1Q&s=FIjAFu91y9yQQlNOxBR3MNPtBoxckgBquFpcazgFFcc&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=242281&view=rev</a><br>
Log:<br>
[PM/AA] Fix *numerous* serious bugs in GlobalsModRef found by<br>
inspection.<br>
<br>
While we want to handle calls specially in this code because they should<br>
have been modeled by the call graph analysis that precedes it, we should<br>
*not* be re-implementing the predicates for whether an instruction reads<br>
or writes memory. Those are well defined already. Notably, at least the<br>
following issues seem to be clearly missed before:<br>
- Ordered atomic loads can "write" to memory by causing writes from other<br>
threads to become visible. Similarly for ordered atomic stores.<br>
- AtomicRMW instructions quite obviously both read and write to memory.<br>
- AtomicCmpXchg instructions also read and write to memory.<br>
- Fences read and write to memory.<br>
- Invokes of intrinsics or memory allocation functions.<br>
<br>
I don't have any test cases, and I suspect this has never really come up<br>
in the real world. But there is no reason why it wouldn't, and it makes<br>
the code simpler to do this the right way.<br>
<br>
While here, I've tried to make the loops significantly simpler as well<br>
and added helpful comments as to what is going on.<br>
<br>
Modified:<br>
llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp<br>
<br>
Modified: llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Analysis_IPA_GlobalsModRef.cpp-3Frev-3D242281-26r1-3D242280-26r2-3D242281-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=v_2U7OQd83CL4gchkkSZVU3fb0tMVRRG0j7MxIm2l1Q&s=Q0Y9JUhRV2iEM7NdBl52tiz7eVvdznT8n2zfuYufLF0&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp?rev=242281&r1=242280&r2=242281&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp Wed Jul 15 03:53:29 2015<br>
@@ -439,30 +439,39 @@ void GlobalsModRef::AnalyzeCallGraph(Cal<br>
}<br>
<br>
// Scan the function bodies for explicit loads or stores.<br>
- for (unsigned i = 0, e = SCC.size(); i != e && FunctionEffect != ModRef;<br>
- ++i)<br>
- for (inst_iterator II = inst_begin(SCC[i]->getFunction()),<br>
- E = inst_end(SCC[i]->getFunction());<br>
- II != E && FunctionEffect != ModRef; ++II)<br>
- if (LoadInst *LI = dyn_cast<LoadInst>(&*II)) {<br>
+ for (auto *Node : SCC) {<br>
+ if (FunctionEffect == ModRef)<br>
+ break; // The mod/ref lattice saturates here.<br>
+ for (Instruction &I : inst_range(Node->getFunction())) {<br>
+ if (FunctionEffect == ModRef)<br>
+ break; // The mod/ref lattice saturates here.<br>
+<br>
+ // We handle calls specially because the graph-relevant aspects are<br>
+ // handled above.<br>
+ if (auto CS = CallSite(&I)) {<br>
+ if (isAllocationFn(&I, TLI) || isFreeCall(&I, TLI)) {<br>
+ // FIXME: It is completely unclear why this is necessary and not<br>
+ // handled by the above graph code.<br>
+ FunctionEffect |= ModRef;<br>
+ } else if (Function *Callee = CS.getCalledFunction()) {<br>
+ // The callgraph doesn't include intrinsic calls.<br>
+ if (Callee->isIntrinsic()) {<br>
+ ModRefBehavior Behaviour =<br>
+ AliasAnalysis::getModRefBehavior(Callee);<br>
+ FunctionEffect |= (Behaviour & ModRef);<br>
+ }<br>
+ }<br>
+ continue;<br>
+ }<br>
+<br>
+ // All non-call instructions we use the primary predicates for whether<br>
+ // thay read or write memory.<br>
+ if (I.mayReadFromMemory())<br>
FunctionEffect |= Ref;<br>
- if (LI->isVolatile())<br>
- // Volatile loads may have side-effects, so mark them as writing<br>
- // memory (for example, a flag inside the processor).<br>
- FunctionEffect |= Mod;<br>
- } else if (StoreInst *SI = dyn_cast<StoreInst>(&*II)) {<br>
+ if (I.mayWriteToMemory())<br>
FunctionEffect |= Mod;<br>
- if (SI->isVolatile())<br>
- // Treat volatile stores as reading memory somewhere.<br>
- FunctionEffect |= Ref;<br>
- } else if (isAllocationFn(&*II, TLI) || isFreeCall(&*II, TLI)) {<br>
- FunctionEffect |= ModRef;<br>
- } else if (IntrinsicInst *Intrinsic = dyn_cast<IntrinsicInst>(&*II)) {<br>
- // The callgraph doesn't include intrinsic calls.<br>
- Function *Callee = Intrinsic->getCalledFunction();<br>
- ModRefBehavior Behaviour = AliasAnalysis::getModRefBehavior(Callee);<br>
- FunctionEffect |= (Behaviour & ModRef);<br>
- }<br>
+ }<br>
+ }<br>
<br>
if ((FunctionEffect & Mod) == 0)<br>
++NumReadMemFunctions;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>