[PATCH] D53203: Allow MemoryLocation to carry pre-existing knowledge to AA to elide expensive repeated checks

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked an inline comment as done.
dsanders added a comment.

Thanks Alina



================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:822
+        isNonEscapingLocalObject(Object)))) {
 
     // Optimistically assume that call doesn't touch Object and check this
----------------
asbirlea wrote:
> Update flags in Loc here if isNonEscapingLocalObject was called?
> This will make Loc non-const, which is a larger chance in itself and there should be some comments added if we make getModRefInfo have side-effects.
I like the idea of the Loc being updated with things that become known as a result of AA as I'm sure there's plenty more caching that can be done. However, I'm a bit worried about the effects of making the MemoryLocation non-const. My main concern is that callers will find that caching is happening unexpectedly and that passes will therefore cache automatically, modify the IR, but forget to clear the known flags because they weren't aware of the caching. The other concerns are mostly about unintended implicit copies caused by assigning const MemoryLocation to non-const MemoryLocation, and about the size of the patch as dropping the const will likely propagate outwards quite a long way.

I think it would probably be better to add an extra argument to make the caching something the callers have to knowingly opt into. I'm thinking something like:
    ModRefInfo getModRefInfo(ImmutableCallSite CS, const MemoryLocation &Loc, MemoryLocationKnowledge &Knowledge);
    ModRefInfo getModRefInfo(ImmutableCallSite CS, const MemoryLocation &Loc) {
      MemoryLocationKnowledge Knowledge;
      return getModRefInfo(CS, Loc, Knowledge);
    }

That way, callers that aren't aware of it or don't want to preserve it can keep calling:
    getModRefInfo(CS, Loc)
and discarding any cachable knowledge. Those that want full caching (e.g. analysis passes) can do something like:
    getModRefInfo(CS, Loc, Loc.Knowledge)
meanwhile those that need to be more careful can do:
    MemoryLocationKnowledge Knowledge;
    getModRefInfo(CS, Loc, Knowledge)
    ... some transformation ...
    Knowledge.forgetX();
    Knowledge.forgetY();
    Loc.Knowledge = Knowledge;

Does that sound like a good direction to go?


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:774
+        // lots of allocas and lots of noreturn functions (e.g. assert)
+        KnownFlags |= PointerMayBeCaptured(&I, false, true)
+                          ? MLK_KnownMayEscape
----------------
asbirlea wrote:
> Can we avoid this call and leave the Escaping bits both not set?
> Precisely because PointerMayBeCaptured is expensive. Ideally this should be computed only on demand (in geModRefInfo) and the info passed back when caching.
We can, although for DSE at least we won't end up saving any further calls to it as a result. DSE calls getModRef() for every local (or local equivalent) in DeadStackObjects so we'll always call PointerMayBeCaptured() once for each item either way


Repository:
  rL LLVM

https://reviews.llvm.org/D53203