[PATCH] D79454: [IR] `byval` arguments cause reads at call sites

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I'm a little skeptical this is a good idea.  Yes, if you examine the assembly generated by most backends, you'll find that the code which performs the copy is written in the caller.  But we don't promise that in general, and at the IR level we've been modeling the "byval" as part of the callee for a long time.  And the incentive to change this seems weak.

If you are going to change this, please audit all the other places that query the readnone/readonly attributes directly; I'm not convinced you've found them all.  And please document this in LangRef.

> Do we need equivalent handling for the preallocated and (soon to be removed) inalloca attributes?

With inalloca/preallocated, the callee explicitly reads/modifies memory allocated in the caller.  It isn't any different from a normal non-byval pointer parameter in this respect.



================
Comment at: llvm/lib/IR/Instruction.cpp:542
+    return !cast<CallBase>(this)->doesNotReadMemory() ||
+           cast<CallBase>(this)->hasByValArgument();
   case Instruction::Store:
----------------
arsenm wrote:
> It technically reads memory, but does it really read memory from an IR perspective? This isn't supposed to be a program visible read?
In the callee, the parameter points to a distinct memory allocation that can be read/modified independently of the memory pointed by the caller.  This is definitely visible to the program.

Yes, this is a messy edge case of LLVM IR; arguably, it isn't a good idea, and we should kill off byval in favor of explicitly writing out the allocation/copy in the caller (https://reviews.llvm.org/D74651).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79454