<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><hr id="zwchr"><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Daniel Berlin" <dberlin@dberlin.org><br><b>To: </b>reviews+D7864+public+020798531ef64731@reviews.llvm.org<br><b>Cc: </b>"Hal Finkel" <hfinkel@anl.gov>, "Nick Lewycky" <nlewycky@google.com>, "James Molloy" <james.molloy@arm.com>, "Philip Reames" <listmail@philipreames.com>, "Sanjoy Das" <sanjoy@playingwithpointers.com>, "Commit Messages and Patches for LLVM" <llvm-commits@cs.uiuc.edu><br><b>Sent: </b>Friday, February 27, 2015 4:18:46 PM<br><b>Subject: </b>Re: [PATCH] This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h<br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 27, 2015 at 1:18 PM, <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:86<br>
@@ +85,3 @@<br>
+// This is mostly ripped from MemoryDependenceAnalysis, updated to<br>
+// handle call instructions properly<br>
+<br>
----------------<br>
What exactly does "updated to handle call instructions properly" mean? Also, this looks like it should be a member function of AA. Can you please add it there (then we can update MDA to use it too?).<br></blockquote><div><br></div><div>It means MDA's version would crash on call instructions :)</div><div><br></div><div id="DWT5436">Happy to make it part of AA and update both.</div></div></div></div></blockquote><br>I think that's a good idea. Having several only-slightly-different utility functions for this scattered about does not seem optimal.<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:100<br>
@@ +99,3 @@<br>
+ else if (auto *I = dyn_cast<CallInst>(Inst))<br>
+ return AliasAnalysis::Location(I);<br>
+ else if (auto *I = dyn_cast<InvokeInst>(Inst))<br>
----------------<br>
Part of my confusion is this: What does the call's return value have to do with the memory location(s) accessed by the call?<br></blockquote><div><br></div><div>Nothing. But BasicAA crashes on Calls/Invokes if you call getModRefInfo and Loc.Ptr is null. MDA sets is to the call instruction, so we do the same.</div><div><br></div><div>(Note: getModRefInfo ends up using the right call down the line, it creates an immutablecallsite out of it, and uses that vs actual location)</div><div><br></div><div id="DWT5988">You are more than welcome to say "we should go and fix this" :)</div></div></div></div></blockquote><br>Good, we should go and fix this ;)<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
We do have these in AA:<br>
<br>
static Location getLocationForSource(const MemTransferInst *MTI);<br>
static Location getLocationForDest(const MemIntrinsic *MI);<br>
<br>
and I imagine those would be good to use here.<br>
<br>
But for generic calls, we don't have any facility to get a location, or list of locations, touched by the call. We can do call-site vs. call-site queries, and call-site vs. Location queries, however.<br></blockquote><div><br></div><div>It is actually doing a call-site query, the one place this got used was essentially to tell whether a call was readonly/had no memory effects.</div><div><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:136<br>
@@ +135,3 @@<br>
+ // 1. One argument dominates the other, and the other's version is a<br>
+ // "false" one.<br>
+ // 2. All of the arguments are false version numbers that eventually<br>
----------------<br>
Can you please define here what a "false" version is? (non-aliasing?)<br></blockquote><div><br></div><div>Will do (it's non-aliasing)</div><div> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:241<br>
@@ +240,3 @@<br>
+ break;<br>
+ } else if (AA->alias(getLocationForAA(AA, DefMemoryInst), Loc) !=<br>
+ AliasAnalysis::NoAlias)<br>
----------------<br>
I'm likely missing something here, but why don't you just call AA->getModRefInfo for all types of instructions? The overload for Instruction* handles all of the various types:<br></blockquote><div><br></div><div>It doesn't actually work properly on some types of Loc, despite it trying to dispatch everything :)</div><div><br></div><div id="DWT5435">Happy to fix it :)</div></div></div></div></blockquote><br>Yes, please do :)<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
/// getModRefInfo - Return information about whether or not an instruction may<br>
/// read or write the specified memory location. An instruction<br>
/// that doesn't read or write memory may be trivially LICM'd for example.<br>
ModRefResult getModRefInfo(const Instruction *I,<br>
const Location &Loc) {<br>
switch (I->getOpcode()) {<br>
case Instruction::VAArg: return getModRefInfo((const VAArgInst*)I, Loc);<br>
case Instruction::Load: return getModRefInfo((const LoadInst*)I, Loc);<br>
case Instruction::Store: return getModRefInfo((const StoreInst*)I, Loc);<br>
case Instruction::Fence: return getModRefInfo((const FenceInst*)I, Loc);<br>
case Instruction::AtomicCmpXchg:<br>
return getModRefInfo((const AtomicCmpXchgInst*)I, Loc);<br>
case Instruction::AtomicRMW:<br>
return getModRefInfo((const AtomicRMWInst*)I, Loc);<br>
case Instruction::Call: return getModRefInfo((const CallInst*)I, Loc);<br>
case Instruction::Invoke: return getModRefInfo((const InvokeInst*)I,Loc);<br>
default: return NoModRef;<br>
}<br>
}<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:269<br>
@@ +268,3 @@<br>
+ ++NumClobberCacheHits;<br>
+ DEBUG(dbgs() << "Cached Memory SSA version for " << (uintptr_t)I << " is ");<br>
+ DEBUG(dbgs() << (uintptr_t)CCV->second);<br>
----------------<br>
Why are you printing the instruction addresses here? Would it be more useful to print *I?<br></blockquote><div><br></div><div>So, you can't really print anything sane anymore other than that since I removed the otherwise-unused defVersion field in a previous patch. If you look a few versions back in the patch, it used to print<br></div><div><br></div><div>"<version> = MemoryDef(<version>)" if you printed *I, and the operator override would print the version here, so you'd get </div><div><br><version> used to come from the defVersion member of the structure.</div><div><br></div><div>defVersion is superfluous, however, since they are really linked the same way LLVM instructions/values are linked -> memory pointer to memory pointer.</div><div>It was suggested i remove it, so i did. This means you don't know what <version> should be, outside of a memory address, unless you make it up.</div><div><br></div><div id="DWT5989">The only way to make a "nice" printout now is to do what the annotation writer does, which is assign each memoryaccess* unique numbers and use them consistently (like slottracker does)</div></div></div></div></blockquote>Okay.<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:651<br>
@@ +650,3 @@<br>
+ Loc.Ptr = BI;<br>
+ AliasAnalysis::ModRefResult ModRef = AA->getModRefInfo(BI, Loc);<br>
+ if (ModRef & AliasAnalysis::Mod)<br>
----------------<br>
You're testing against an empty location?</blockquote><div><br></div><div>Yes. The goal is to figure out whether the instruction can have memory side-effects. If you look at AliasAnalysis.cpp, that is, in fact, what it does with an empty Loc :)</div><div><br></div><div id="DWT6143">Better suggestions welcome, but duplicating all the functionality it has in there to know things about load ordering flags, etc, seemed a bad idea.</div></div></div></div></blockquote><br>I don't want to duplicate that functionality, but I also don't want to work-around a insufficiently-expressive AA interface. And regardless, I certainly don't want to embed these kinds of AA implementation details in other passes.<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><div>Note that this crashes on call if Loc.ptr is null, but does not crash on any other type of instruction if loc.ptr is null.</div><div><br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> I don't think this is what you want, how about using this:<br>
<br>
/// getModRefBehavior - Return the behavior when calling the given call site.<br>
virtual ModRefBehavior getModRefBehavior(ImmutableCallSite CS);<br></blockquote><div><br></div><div><br></div><div>This won't work for anything but calls.</div><div>This loop is basically trying to figure out where to place defs. it needs to place defs anywhere something can actually write to memory, no matter what the instruction type, etc.</div><div><br></div><div id="DWT6144">It is using getModRefInfo to get this behavior.</div></div></div></div></blockquote><br>Sure, but my point was that you're already special-casing call sites, so why not just use getModRefBehavior for these cases. Perhaps better yet, we can fix the AA implementation to deal with an empty Location.<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:688<br>
@@ +687,3 @@<br>
+ // arguments or users of globals. We do not actually insert it in to<br>
+ // the IR.<br>
+ BasicBlock &StartingPoint = F.getEntryBlock();<br>
----------------<br>
But you don't really insert anything into the IR proper. I'd rephrase this.<br></blockquote><div>will do</div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><br></blockquote></div></div></div>
</blockquote>Thanks again,<br>Hal<br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>