[PATCH] D62766: [Attributor] Deduce "nosync" function attribute.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1481-1488
+``nosync``
+    This function attribute indicates that the function does not communicate
+    (synchronize) with another thread. By synchronization we mean that the function
+    does not access memory through non-relaxed atomic instructions or does not use
+    volatile acesses. This should be distinguished from cross-lane operations,
+    which could be interpreted as kind of synchronization is where threating it
+    as a memory dependence is not sufficient. If the function does ever synchronize
----------------
jdoerfert wrote:
> arsenm wrote:
> > jdoerfert wrote:
> > > nhaehnle wrote:
> > > > Thanks, I think this is better, but there are still some problems:
> > > > * There are no relaxed atomics in LLVM, only unordered, monotonic, and stronger orderings.
> > > > * What about fences?
> > > > 
> > > > I would put the part about cross-lane operations at the end and rephrase it slightly for clarity. Suggestion:
> > > > 
> > > > > This attribute is only concerned with synchronization through memory operations and is therefore orthogonal to cross-lane and convergent operations. In particular, an operation such as a barrier can be `convergent` but also `nosync`.
> > > > 
> > > > Assuming we can agree about the actual statement of that last sentence...
> > > >  >   This attribute is only concerned with synchronization through memory operations and is therefore orthogonal to cross-lane and convergent operations. In particular, an operation such as a barrier can be convergent but also nosync.
> > > 
> > > > Assuming we can agree about the actual statement of that last sentence...
> > > 
> > > This proposed change, and the one requested earlier and integrated (sync goes through memory), are problematic.
> > > I first though they are fine but they will probably make the attribute unusable.
> > > 
> > > An alternative proposal would be:
> > > 
> > > ```
> > > This function attribute indicates that the function does not communicate
> > > (synchronize) with another thread through memory or other well-defined means.
> > > Synchronization is considered possible in the presence of `atomic` accesses that enforce an order, thus not "unordered" and "monotonic", `volatile` accesses, as well as `convergent` function calls. Note that through the latter non-memory communication, e.g., cross-lane operations, is also considered synchronization. If an annotated function does ever synchronize with another thread, the behavior is undefined.
> > > ```
> > > 
> > > If this is where we are heading, we need to make sure we test:
> > > 1) `non-convergent` does not allow `nosync`, e.g.,  `readnone` does not imply `nosync`
> > > 2) `readnone` and `non-convergent` does imply `nosync`
> > > 
> > > @arsenm, @nhaehnle. @jfb, what do you think?
> > What makes it unusable exactly?
> > 
> > This wording confuses me:
> > 
> > > Note that through the latter non-memory communication, e.g., cross-lane operations, is also considered synchronization. 
> > 
> > I'm not 100% comfortable specifically referring to convergent, since I'm still worried about the yet-to-be-defined anticonvergent attribute. Though it is hard to define something around an unsolved problem. 
> > 
> > This phrasing also implies to me that call site merging is not legal, which is what I thought you were trying to avoid. 
> > 
> > Conclusion 2 sounds OK to me. Conclusion 1 sounds like the opposite of what the goal is?
> > What makes it unusable exactly?
> 
> `nosync` would then still allow non-memory synchronization which it shouldn't.
> 
> I think the IRC conversation helped. What I want us to have is:
> 
> `nosync` means **no** synchronization/communication between "threads". Any potential synchronization, e.g., through memory or registers, precludes `nosync`.
> 
> I think the IRC conversation helped.

Is that recorded somewhere?

> `nosync` would then still allow non-memory synchronization which it shouldn't.

This is questionable. There are `convergent` operations that do not imply synchronization. For example, some of the `llvm.amdgcn.image.sample.*` intrinsics are convergent, but they do not imply any kind of synchronization in the memory model.

In Vulkan/SPIR-V parlance, the intrinsic may have an implied //control// barrier, but it definitely has no //memory// barrier (the control barrier part isn't fully spec'd out in SPIR-V either at the moment).

For the initial intended usage of this attribute: if there is a pointer that you know to be dereferencable before the image sample, then you still know it to be dereferencable afterwards. So it seems reasonable to want the intrinsic to be marked both `convergent` and `nosync`.

That said, I'm okay with this part of it:

> If this is where we are heading, we need to make sure we test:
> 1.  non-convergent does not allow nosync, e.g., readnone does not imply nosync
> 2.  readnone and non-convergent does imply nosync

... so long as it's understood that those are "merely" the rules for the attributor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766