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

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

You should also add a test function with inline assembly.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:315
+    AtomicOrdering Failure = cast<AtomicCmpXchgInst>(I)->getFailureOrdering();
+    // Only if both are relaxed, than it can be treated as relaxed.
+    // Otherwise it is non-relaxed.
----------------
"then"


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:378
+  /// We are looking for volatile instructions or Non-Relaxed atomics.
+  /// FIXME: We should ipmrove the handling of intrinsics.
+  for (Instruction *I : InfoCache.getReadOrWriteInstsForFunction(F)) {
----------------
"improve"


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766