<div dir="ltr">Thanks for the explanations/details - not too fussed about any of it, if there's arguments to be made for consistency, etc, as it suits you. :)</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Feb 27, 2017 at 9:35 AM Daniel Berlin via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dberlin marked 2 inline comments as done.<br class="gmail_msg">
dberlin added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/Analysis/MemoryLocation.h:73-74<br class="gmail_msg">
+ const Optional<MemoryLocation> &Loc = MemoryLocation::getOrNone(Inst);<br class="gmail_msg">
+ assert(Loc.hasValue() && "unsupported memory instruction");<br class="gmail_msg">
+ return Loc.getValue();<br class="gmail_msg">
+ }<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Optional already has the assert in it, so unless you particularly want that assert text/message, you could write this as:<br class="gmail_msg">
><br class="gmail_msg">
> return *MemoryLocation.getOrNone(Inst);<br class="gmail_msg">
I was trying to duplicate the exact functionality of the existing code, but happy to change it.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/Analysis/MemoryLocation.h:77-86<br class="gmail_msg">
if (auto *I = dyn_cast<LoadInst>(Inst))<br class="gmail_msg">
return get(I);<br class="gmail_msg">
else if (auto *I = dyn_cast<StoreInst>(Inst))<br class="gmail_msg">
return get(I);<br class="gmail_msg">
else if (auto *I = dyn_cast<VAArgInst>(Inst))<br class="gmail_msg">
return get(I);<br class="gmail_msg">
else if (auto *I = dyn_cast<AtomicCmpXchgInst>(Inst))<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Consider a switch over the inst kind enum to reduce the work repeatedly done by dyn_cast (at least it seems like LLVM code often uses a switch over enum rather than chained dyn_cast - I don't actually know if the overhead is real)<br class="gmail_msg">
So i didn't write this, and i'm happy to clean it up in a future patch, but i want to not touch it here if that's okay, as i would like the patch to change as little as possible :)<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
If you stare at MemoryLocation.cpp, you'll see that basically all of these are doing the same thing, and probably could stand a bit of templating.<br class="gmail_msg">
<br class="gmail_msg">
Honestly, at this point, we may just want to make the memory instructions able to return a memorylocation in Instructions.h.<br class="gmail_msg">
Who knows.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Transforms/Utils/MemorySSA.cpp:149<br class="gmail_msg">
<br class="gmail_msg">
+ const Optional<MemoryLocation> &getLocOrNone() const { return Loc; }<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Should this assert !IsCall?<br class="gmail_msg">
No.<br class="gmail_msg">
Actually, we expect calls to return None here.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Transforms/Utils/MemorySSA.cpp:162-163<br class="gmail_msg">
union {<br class="gmail_msg">
ImmutableCallSite CS;<br class="gmail_msg">
- MemoryLocation Loc;<br class="gmail_msg">
+ Optional<MemoryLocation> Loc;<br class="gmail_msg">
};<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Does !IsCall imply that Loc.hasValue() ? (or can the "None" state of Loc be used in some cases) Judging by "getLoc" I'm guessing it does imply that.<br class="gmail_msg">
><br class="gmail_msg">
> If it does imply that, then Optional's hasValue tracking is wasted space. Wonder if there's a nice abstraction that should exist for this case, but I'm not sure.<br class="gmail_msg">
><br class="gmail_msg">
> I ask partly because the use of Optional maybe makes that invariant non-obvious so a better matched abstraction may make the code easier to understand.<br class="gmail_msg">
"If it does imply that, then Optional's hasValue tracking is wasted space. Wonder if there's a nice abstraction that should exist for this case, but I'm not sure."<br class="gmail_msg">
Yes, it's a bit of waste space. We could have a traits that lets you reuse a bit in the thing you are making optional, and use that. That is how we do a lot of "use this thing, but storage is external" in LLVM.<br class="gmail_msg">
But not sure it's worth it.<br class="gmail_msg">
<br class="gmail_msg">
In reality, this entire class is working around a bit of api deficiency, IMHO.<br class="gmail_msg">
<br class="gmail_msg">
We want ways to make "hashtable of memory locations", but calls don't have memory locations (or more accurately, they have multiple ones).<br class="gmail_msg">
<br class="gmail_msg">
IE you really want to be able to say "the last time i saw this abstract memory area, it had this value".<br class="gmail_msg">
<br class="gmail_msg">
But we only can do that for non-call instructions.<br class="gmail_msg">
So either we have one mapping for calls, and none for non-calls, which ends up a different kind of ugly, or we have a class like this that tries to encapsulate the difference.<br class="gmail_msg">
<br class="gmail_msg">
In practice, all of this should go away at some point, and we should come up with a clean API that allows mapping/caching memory locations to other things, without the current mess, because nobody should have to care about this :)<br class="gmail_msg">
<br class="gmail_msg">
Even if that is really "MLocOrCall becomes the new MemoryLocation".<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D30369" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D30369</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div>