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

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 3 15:04:48 PDT 2019


jdoerfert added inline comments.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:293-295
+; TEST 14 - negative, checking volatile intrinsics.
+
+; ATTRIBUTOR: define i32 @memcpy_volatile(i8* %ptr1, i8* %ptr2)
----------------
sstefan1 wrote:
> nhaehnle wrote:
> > The negative check line here is missing.
> I skipped the negative check line, because for now only nosync is deduced and if function is not nosync there will be no Function Attrs at all. Once we have some more attributes, I'll add the negative check. Is that ok with you?
Just give the function the `nounwind` attribute and then add the negative check line.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:317
+  ret i32 4
+}
----------------
sstefan1 wrote:
> jdoerfert wrote:
> > 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.
> Replaced llvm.cos test with inline assembly. llvm.cos test was my mistake, since with the current implementation it would be considered sync.
The test comment is off, and again add `nounwind` to allow for the check lines


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766





More information about the llvm-commits mailing list