[PATCH] D97924: [LangRef] clarify the semantics of nocapture

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Minor comments, this looks like it's evolved into a reasonable step forward.  As things stand, I think this should go in with some minor stuff fixed, and we can iterate on details in further patches.  This is already better than what we have now.  :)



================
Comment at: llvm/docs/LangRef.rst:1206
+    pointer. This is not a valid attribute for return values.
+    A caller can pass two same pointers with one nocapture tagged and another
+    not tagged as arguments.
----------------
aqjune wrote:
> I moved the volatile thing to the new section.
I suggest tweaking this wording slightly.  Specifically:

"This attribute applies only to the particular copy of the pointer passed in this argument.  A caller could pass two copies of the same pointer with one being annotated nocapture and the other not, and the callee could validly capture through the non annotated parameter.

(Great example btw.)


================
Comment at: llvm/docs/LangRef.rst:2671
+that is readable by the caller or subsequent function calls, such as a global
+variable or the caller's register.
+
----------------
nlopes wrote:
> let's make it explicit the pointer only escapes if that information is readable by the caller after the function exits. To account for the fact that information can be overwritten.
> Maybe complement the example below to have something like:
>    store i32 %j, i32* @glb ; escapes %a
>    store i32 0, i32* @glb ; %a is not escaped anymore
JFYI, this example is very subtle and possibly misleading.  It's only correct since both stores are non-atomic.

If there was another threading reading from @glb, then it could capture the value of %a between the two stores.

However, that would require a race which would be UB (since the stores aren't atomic) and thus we can ignore it.

Importantly, if there was any ordering operation between the two stores it might capture, even with non-atomic stores.


================
Comment at: llvm/docs/LangRef.rst:2700
+
+Addresses used in volatile operations are considered to be captured.
+
----------------
Looks like this should be 3).


================
Comment at: llvm/docs/LangRef.rst:2703
+
 .. _volatile:
 
----------------
You should add an item about the inspection by another thread case.

(This case gets subtle and complicated fast.  I'd suggest using deliberately hand wavy wording to start with and refining that in a follow up patch.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97924