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

[David Blaikie via llvm-commits]

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

On Mon, Feb 27, 2017 at 9:35 AM Daniel Berlin via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170227/a3a36752/attachment.html>