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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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
----------------
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`.



================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:317
+  ret i32 4
+}
----------------
jfb wrote:
> jdoerfert wrote:
> > jfb wrote:
> > > I don't think you can generally treat intrinsics as `nosync`. Unless you know they're actually `nosync` you should assume that intrinsics might synchronize.
> > > 
> > > For example:
> > > ```
> > > int a;
> > > void i_totally_sync() {
> > >  __builtin_ia32_clflush(&a);
> > > }
> > > ```
> > > Corresponds to:
> > > ```
> > > tail call void @llvm.x86.sse2.clflush(i8* bitcast (i32* @a to i8*))
> > > ```
> > > You should have a test for this, and it should definitely *not* be `nosync`.
> > > 
> > > The other option here is to go and add a field to all intrinsics, so when creating a new one we have to figure out whether it'll definitely sync, maybe sync, or never sync. I don't think that's in scope for this patch.
> > > I don't think you can generally treat intrinsics as nosync. Unless you know they're actually nosync you should assume that intrinsics might synchronize.
> > 
> > Good point. Maybe the best way (for now and in general) is to "not look for" intrinsics. Use the same logic for all instructions. That is, if it is a call and not annotated as no-sync it may-sync. The test with `llvm.cos` can be adjusted by adding `readnone` to  the decleration of`llvm.cos`.
> > 
> > 
> > > The other option here is to go and add a field to all intrinsics, so when creating a new one we have to figure out whether it'll definitely sync, maybe sync, or never sync. I don't think that's in scope for this patch.
> > 
> > Agreed, we will have to do that for various attributes at some point (soon) but not in this patch.
> > 
> > 
> I'd still accept the volatile `mem*` intrinsics as is already done, but otherwise yeah intrinsics should be assumed to synchronize.
Agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766