[PATCH] D121415: [FunctionAttrs] Infer argmemonly .

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:192
             MemoryLocation::getBeforeOrAfter(Arg, I.getAAMetadata());
+        AccessesNonArgsOrAlloca |= !IsArgumentOrAlloca(Loc.Ptr);
 
----------------
nikic wrote:
> Hm, should these checks be going below the pointsToConstantMemory() checks, for the same reason why we don't consider a load from constant memory as a "ref"?
> 
> As an aside, pointsToConstantMemory() already does a recursive underlying object walk to check whether the pointer is based on a constant global or alloca, and this adds an additional (weaker, non-recursive) walk. It would probably make sense to convert the underlying pointerToConstantMemory() API into something like classifyLocation() that can return one of Constant, Local, Argument or Unknown, so all of these can be handled in a uniform manner.
I moved the check below `AAR.pointsToConstantMemory(` in all cases below.

> It would probably make sense to convert the underlying pointerToConstantMemory() API into something like classifyLocation() that can return one of Constant, Local, Argument or Unknown, so all of these can be handled in a uniform manner.

Sounds good. Are you aware of any other places that would benefit from a more general version?


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:296
+  // argmemonly, exit.
+  if (ReadsMemory && WritesMemory && !ArgMemOnly)
     return;
----------------
nikic wrote:
> I think this check if redundant, as we already check the same thing inside the loop.
Thanks, I replaced it with an assertion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121415/new/

https://reviews.llvm.org/D121415