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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D97924#2607261 <https://reviews.llvm.org/D97924#2607261>, @nlopes wrote:

>> If we have an object which has not yet been captured passed to a nocapture argument of a function, we know that the object remains uncaptured after the call.  Additionally, we also know that the callee hasn't increased the number of locations in which references to the object can be observed after the call.  Thus, if the caller can precisely enumerate said set before the call, that said remains precise after the call completes.
>
> You are hinting at a more dynamic semantics, while we're proposing a more syntactic one.

Yes I am, and I think that's important.  Its worth noting that every syntactic definition has a corresponding dynamic version, but having the dynamic one be simple and easy to understand is important.

Key reasons:

1. I personally have a hard time understanding syntactic definitions.  Normally, I'd hesitate to insert my personal preferences so strongly, but as one of the people who has historical fixed bugs in this area, this actually an important point.
2. My experience is that the dynamic definitions are easier to explain.
3. Dynamic definitions are easier to test.  In particular, the map themselves well to a collection of test cases and even sanitizers.  I think the last point (sanitizers) is actually quite important because as we become more aggressive about optimizing based on capture semantics I expect we'll find existing frontend bugs in usage.

> Consider this example:
>
>   f(nocapture p) {
>     p2 = load glb
>     ret p2
>   }
>
> According to your definition, this functions triggers UB if p2 == p, because it increases the number of observations of p. It's counter-intuitive to me that a function that doesn't touch its nocapture argument captures that argument.

I agree, this is slightly counter intuitive.  I think it's also fundamentally necessary.

One of the key optimization properties we want is that a nocapture *and trackable* pointer remains nocapture *and trackable* after being passed to a nocapture argument.  Given we don't have a way to describe in current langref the newly created copy, we must consider this a violation of the existing nocapture parameter property.

Note the distinction in my wording between nocapture and "trackable".  The existing nocapture attribute mixes two related, but distinct, properties.

> Another case is this:
>
>   f(nocapture p, q) {
>     glb = g
>   }
>
> Similarly, this function would trigger UB if p == q. This contrasts with how other attributes work. For example, if a pointer is readonly we can't write through it, but we can still write to the object provided we have another non-readonly pointer to it.

I think for this example, you're looking at the stale wording from the review rather than the updated version in the linked to document.  I agree we want pointer properties, not object ones, and attempted to address that in the wording.  Does the new wording match what you'd expect?

The "trackability" part my current definition for the argument attribute does seem more object based, and I agree that needs some word smithing.  I'm not arguing in favor of an object semantics here, it simply happens to be the way my brain processes this stuff and it tends to slide into my wording if I'm not careful.

> We've been thinking along these lines:
>
> - a function cannot return a pointer tagged as nocapture
> - on return, the memory cannot have any nocapture-tagged pointer stored in captured objects, global objects, or objects passed as arguments
>
> It's a more syntactic view than what you are proposing, but I think it's more in line with the other attributes.

I'll just point out that with this definition, we still need to define what a captured object is.  That's the root point I'm trying to get defined, everything else builds on top.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97924