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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766