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

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766