[PATCH] D121415: [FunctionAttrs] Infer argmemonly .

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:145
+    const Value *UO = getUnderlyingObject(Ptr);
+    return UO && (isa<Argument>(UO) || isa<AllocaInst>(UO));
+  };
----------------
getUnderlyingObject() never returns null.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:192
             MemoryLocation::getBeforeOrAfter(Arg, I.getAAMetadata());
+        AccessesNonArgsOrAlloca |= !IsArgumentOrAlloca(Loc.Ptr);
 
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121415