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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D79454#2023110 <https://reviews.llvm.org/D79454#2023110>, @efriedma wrote:

> > EDIT: It seems Clang actually already performs the thing I describe below o.O.
>
> Yes, like I mentioned, we have a long history of making the current semantics work. :)
>
> I think I can see that changing the semantics like this makes the overall model a little cleaner.  But do we actually get any concrete benefit from it?  I don't think this actually makes Attributor more powerful; it just changes the way we represent the end result a bit.


Not more powerful no (see EDIT below). It makes it more intuitive I'd say. For one, not only the Attributor but also other passes would right now assume `readnone` for an empty function. Clang might not but run my example with `O1` (https://godbolt.org/z/CjxNhN) to see how we infer `readnone`. That said, it is clear there is inconsistency. I would propose to adapt the language reference first. 
I still believe this patch (=this model) is best suited to get consistent semantics that work. We can continue the intuitive `readnone` deduction for empty functions but if a call with a `byval` argument is present it is not considered `readnone` for everyone using the `Instruction::mayReadMemory` interface. Basically, if you look at attributes yourself you have to look at all of them. The only alternative I can see is to avoid `readnone` for functions with `byval` arguments everywhere. I would assume that is more complicated and a burden on downstream users as well. Also, we don't do that right now anyway ;)

EDIT: If we would go with the `byval` prevents `readnone` route, we really need the verifier to check that.
EDIT: We might actually loose out if we prevent `readnone` in the presence of `byval` arguments if, the user provided the `readnone/ony` annotation, or it was generated by something like HTO (or maybe part of a LTO summary). Basically whenever we cannot rediscover the fact via IPO and not inline. So having `readnone`, meaning no accesses to non-local memory that could be observed by the caller, seems reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79454