[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
----------------
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?


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:317
+  ret i32 4
+}
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766