[PATCH] D30369: Allow None as a MemoryLocation to getModRefInfo, use it to start cleaning up interfaces and uses

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 2 inline comments as done.
dberlin added inline comments.


================
Comment at: include/llvm/Analysis/MemoryLocation.h:73-74
+    const Optional<MemoryLocation> &Loc = MemoryLocation::getOrNone(Inst);
+    assert(Loc.hasValue() && "unsupported memory instruction");
+    return Loc.getValue();
+  }
----------------
dblaikie wrote:
> Optional already has the assert in it, so unless you particularly want that assert text/message, you could write this as:
> 
>   return *MemoryLocation.getOrNone(Inst);
I was trying to duplicate the exact functionality of the existing code, but happy to change it.



================
Comment at: include/llvm/Analysis/MemoryLocation.h:77-86
     if (auto *I = dyn_cast<LoadInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<StoreInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<VAArgInst>(Inst))
       return get(I);
     else if (auto *I = dyn_cast<AtomicCmpXchgInst>(Inst))
----------------
dblaikie wrote:
> 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)
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 :)


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.

Honestly, at this point, we may just want to make the memory instructions able to return a memorylocation in Instructions.h.
Who knows.




================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:149
 
+  const Optional<MemoryLocation> &getLocOrNone() const { return Loc; }
+
----------------
dblaikie wrote:
> Should this assert !IsCall?
No.
Actually, we expect calls to return None here.



================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:162-163
   union {
     ImmutableCallSite CS;
-    MemoryLocation Loc;
+    Optional<MemoryLocation> Loc;
   };
----------------
dblaikie wrote:
> 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.
> 
> 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.
> 
> 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. 
"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."
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.
But not sure it's worth it.

In reality, this entire class is working around a bit of api deficiency, IMHO.

We want ways to make "hashtable of memory locations", but calls don't have memory locations (or more accurately, they have multiple ones).

IE you really want to be able to say "the last time i saw this abstract memory area, it had this value".

But we only can do that for non-call instructions.
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.

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 :)

Even if that is really "MLocOrCall becomes the new MemoryLocation".



https://reviews.llvm.org/D30369