[PATCH] D73426: [Attributor] Derive memory location attributes (argmemonly, ...)

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

In D73426#1875653 <https://reviews.llvm.org/D73426#1875653>, @uenoku wrote:

> Sorry for the delay. I have just completed my bachelor's course so I can take enough time for development.


No worries. I hope that went well!

> I think a state without `NO_UNKOWN_MEM` is semantically identical to a pessimistic fixpoint, right?

In this patch probably true. With D73527 <https://reviews.llvm.org/D73527> users can iterate over the "unknown" accesses and determine their original on their own. Once we give up, we do not guarantee all accesses are tracked.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:6120-6121
+    if (auto *Arg = dyn_cast<Argument>(&V)) {
+      if (Arg->hasByValAttr())
+        updateState(T, NO_LOCAL_MEM, I, &V, Changed);
+      else
----------------
uenoku wrote:
> Why is byval argument regarded as local memory?
This is how I understand it:
  A `byval` makes a copy "on the call edge". The copy lives in the scope of the callee. So the byval argument is actually a locak (stack) copy in the callee which is initialized with the incoming value at the beginning of the function. That's also why we should mark byval call site arguments as read only regardless of their argument counterpart.


================
Comment at: llvm/test/Transforms/Attributor/ArgumentPromotion/fp80.ll:17
 ; CHECK-LABEL: define {{[^@]+}}@run()
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    unreachable
----------------
uenoku wrote:
> Is this change caused by this patch? 
Yes. With this patch `@CaptureAStruct` is known to be `readnone` which makes all operations we delete here side-effect free, thus AAIsDead removes them for us. It was not `readnone` before because there are loads and stores but only to local memory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73426