[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2023-01-12 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri resigned from this revision. lebedev.ri added a comment. Herald added a subscriber: StephenFan. Herald added a project: All. This review seems to be stuck/dead, consider abandoning if no longer relevant. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-11 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. Sure, I'm happy with the second option. > Alignment at the call site might be higher than of the copy, breaking with > the idea that the call site and callee "properties" match. Though, the > attributes can probably be kept in sync if we teach the relevant parts. The

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert added a comment. In D79636#2028479 , @efriedma wrote: > > @efriedma I'm a bit confused. Could you propose some wording so I get a > > feeling where you want to go? > > Depending on which direction we go, either: > > - "Attributes on a function

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. > @efriedma I'm a bit confused. Could you propose some wording so I get a > feeling where you want to go? Depending on which direction we go, either: - "Attributes on a function or callsite describe the behavior of the callee excluding the implied copy. For example,

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/docs/LangRef.rst:1052-1053 +memory of the callee. That means, a callee can write a ``byval`` parameter +and still be ``readonly`` or ``readnone`` because the write is, similar

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert added a comment. In D79636#2028294 , @efriedma wrote: > > Do you object to say that the call site argument and the argument point to > > distinct memory locations or something else? > > Like I said, my issue is with the "Attributes on the call

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Matt Arsenault via Phabricator via cfe-commits
arsenm added inline comments. Comment at: llvm/docs/LangRef.rst:1052-1053 +memory of the callee. That means, a callee can write a ``byval`` parameter +and still be ``readonly`` or ``readnone`` because the write is, similar to +a write to an ``alloca``, not visible

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Juneyoung Lee via Phabricator via cfe-commits
aqjune added a comment. I have a minor question: > a call of a readnone function with a byval argument is not classified as > readnone (which it is today: https://godbolt.org/z/dDfQ5r) %0 at caller has readnone attribute - is it related with the propagation of readnone attribute from %0 of

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. > Do you object to say that the call site argument and the argument point to > distinct memory locations or something else? Like I said, my issue is with the "Attributes on the call site argument and function argument are associated with the original and copied memory

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert updated this revision to Diff 263026. jdoerfert added a comment. Minor wording tweak Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79636/new/ https://reviews.llvm.org/D79636 Files: llvm/docs/LangRef.rst Index: llvm/docs/LangRef.rst

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert updated this revision to Diff 263024. jdoerfert marked 3 inline comments as done. jdoerfert added a comment. Fix spelling Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79636/new/ https://reviews.llvm.org/D79636 Files:

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert added a comment. > "Attributes on the call site argument and function argument are associated > with the original and copied memory respectively" > > This seems to fly in the face of existing practice, which is that function > attributes are copied to each callsite. I'd strongly

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-09 Thread Reid Kleckner via Phabricator via cfe-commits
rnk added a comment. Some copy editing comments, but I agree with the semantics: From the IR perspective, it is better to think of argument stack memory as belonging to the callee. A byval argument has more in common with a local static alloca than a passed in pointer.

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-08 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added inline comments. Comment at: llvm/docs/LangRef.rst:1050 +the call site argument and function argument are associated with the +original and copied memory respectively. The copy is considered to be local +memory of the callee. That means, a callee can

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

2020-05-08 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert created this revision. jdoerfert added reviewers: arsenm, spatel, efriedma, lebedev.ri, fhahn, rnk, hfinkel, regehr, nlopes. Herald added subscribers: dmgreen, bollu, wdng. Herald added a project: LLVM. After the discussion in D79454 it seemed clear