[IR] Memory intrinsics are not unconditionally `nosync`

[nhaehnle]

Summary

Remove the `nosync` attribute from the memory intrinsic definitions
(i.e. memset, memcpy, memmove).

Like native memory accesses, memory intrinsics can be volatile. This is
indicated by an immarg in the intrinsic call. All else equal, a volatile
memory intrinsic is `sync`, so we cannot annotate the intrinsic functions
themselves as `nosync`. The attributor and function-attr passes know to
take the volatile bit into account.

Since `nosync` is a default attribute, this means we have to stop using
the DefaultAttrIntrinsic tablegen class for memory intrinsics, and
specify all default attributes other than `nosync` explicitly.

Most of the test changes are trivial churn, but one test case
(in nosync.ll) was in fact incorrect before this change.

[nhaehnle]

Fired too quickly, the Attributor actually had a bug and an incorrect lit test. This version should fix that.

[jdoerfert]

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

1337

This doesn't make sense to me now. I think I figured out my problem (below) but I wanted to keep my reasoning:
So, a memintrinsic is either nosync or it is not. Let's play this through:

1. We add nosync to the ones that are not volatile we don't need this because of the check below. This doesn't seem to be the case right now.
2. We don't add nosync to any memintrinsics and we need to check them explicitly, this is what the old code was supposed to do (I imagine) but the new code doesn't do that. That said, we add nosync so it's actually 3).
3. We add nosync to all memintrinsics regardless if they are volatile or not. This seems to be plain wrong. The new code will filter the ones out that we wrongly annotated so the error does not propagate. I doubt this is the right strategy though. We should go with 1) or 2) instead.

llvm/test/Transforms/Attributor/nosync.ll

419

FWIW, this looks like a bug to me too.

[reames]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

1519

This removal breaks our ability to infer nosync on a non-volatile memset, exactly as the comment describes. There should be a test for this, if there isn't, let me know and I'll add one.

[jdoerfert]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

1519

I think what happened is:

MemXXX was not marked nosync before, we had to derive it explicitly. To be correct, we checked for the volatile flag. Here and in the Attributor code above.
Then the DefaultAttrsIntrinsic change came and memXXX was marked nosync regardless of the volatile flag. That is wrong as I stated in my comment above. In the Attributor code above this patch now filters the wrongly annotated calls, in this code it doesn't and they just split through and propagate the wrong nosync flag further. Intrinsics.td need to be changed.

[nhaehnle]

Invert the logic of the patch: mem intrinsics shouldn't be unconditionally marked nosync in the first place.

[jdoerfert]

LGTM, let @reames should also comment.

[reames]

Also LGTM, thanks!

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

 //===- AttributorAttributes.cpp - Attributes for Attributor deduction -----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
struct AANoSyncImpl : AANoSync {
   ChangeStatus updateImpl(Attributor &A) override;
 
   /// Helper function used to determine whether an instruction is non-relaxed
   /// atomic. In other words, if an atomic instruction does not have unordered
   /// or monotonic ordering
   static bool isNonRelaxedAtomic(Instruction *I);
 
   /// Helper function specific for intrinsics which are potentially volatile
-  static bool isNoSyncIntrinsic(Instruction *I);
+  static bool isSyncIntrinsic(Instruction *I);
 };
 
 bool AANoSyncImpl::isNonRelaxedAtomic(Instruction *I) {
   if (!I->isAtomic())
     return false;
 
   if (auto *FI = dyn_cast<FenceInst>(I))
     // All legal orderings for fence are stronger than monotonic.
default:
     llvm_unreachable(
         "New atomic operations need to be known in the attributor.");
   }
 
   return (Ordering != AtomicOrdering::Unordered &&
           Ordering != AtomicOrdering::Monotonic);
 }
 
-/// Return true if this intrinsic is nosync.  This is only used for intrinsics
-/// which would be nosync except that they have a volatile flag.  All other
-/// intrinsics are simply annotated with the nosync attribute in Intrinsics.td.
-bool AANoSyncImpl::isNoSyncIntrinsic(Instruction *I) {
+/// Return true if this intrinsic is sync (not-nosync).  This is only used for
+/// intrinsics which have a volatile flag.
+bool AANoSyncImpl::isSyncIntrinsic(Instruction *I) {
   if (auto *MI = dyn_cast<MemIntrinsic>(I))
-    return !MI->isVolatile();
+    return MI->isVolatile();
   return false;
 }
 
 ChangeStatus AANoSyncImpl::updateImpl(Attributor &A) {
 
   auto CheckRWInstForNoSync = [&](Instruction &I) {
     /// We are looking for volatile instructions or Non-Relaxed atomics.
 
     if (const auto *CB = dyn_cast<CallBase>(&I)) {
-      if (CB->hasFnAttr(Attribute::NoSync))
-        return true;
+      if (isSyncIntrinsic(&I))
+        return false;
 
-      if (isNoSyncIntrinsic(&I))
+      if (CB->hasFnAttr(Attribute::NoSync))
         return true;
 
       const auto &NoSyncAA = A.getAAFor<AANoSync>(
           *this, IRPosition::callsite_function(*CB), DepClassTy::REQUIRED);
       return NoSyncAA.isAssumedNoSync();
     }
 
     if (!I.isVolatile() && !isNonRelaxedAtomic(&I))

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

 //===- FunctionAttrs.cpp - Pass which marks functions attributes ----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
static bool InstrBreaksNoSync(Instruction &I, const SCCNodeSet &SCCNodes) {
   auto *CB = dyn_cast<CallBase>(&I);
   if (!CB)
     // Non call site cases covered by the two checks above
     return false;
 
   if (CB->hasFnAttr(Attribute::NoSync))
     return false;
 
-  // Non volatile memset/memcpy/memmoves are nosync
-  // NOTE: Only intrinsics with volatile flags should be handled here.  All
-  // others should be marked in Intrinsics.td.
-  if (auto *MI = dyn_cast<MemIntrinsic>(&I))
-    if (!MI->isVolatile())
-      return false;
-
   // Speculatively assume in SCC.
   if (Function *Callee = CB->getCalledFunction())
     if (SCCNodes.contains(Callee))
       return false;
 
   return true;
 }
 

llvm/test/Transforms/Attributor/nosync.ll

 declare void @llvm.memcpy(i8* %dest, i8* %src, i32 %len, i1 %isvolatile)
 declare void @llvm.memset(i8* %dest, i8 %val, i32 %len, i1 %isvolatile)
 
 ; TEST 14 - negative, checking volatile intrinsics.
 
 ; It is odd to add nocapture but a result of the llvm.memcpy nocapture.
 ;
 define i32 @memcpy_volatile(i8* %ptr1, i8* %ptr2) {
-; IS__TUNIT____: Function Attrs: argmemonly nofree nosync nounwind willreturn
+; IS__TUNIT____: Function Attrs: argmemonly nofree nounwind willreturn
 ; IS__TUNIT____-LABEL: define {{[^@]+}}@memcpy_volatile
 ; IS__TUNIT____-SAME: (i8* nocapture nofree writeonly [[PTR1:%.*]], i8* nocapture nofree readonly [[PTR2:%.*]]) #[[ATTR10:[0-9]+]] {
-; IS__TUNIT____-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* noalias nocapture nofree writeonly [[PTR1]], i8* noalias nocapture nofree readonly [[PTR2]], i32 noundef 8, i1 noundef true) #[[ATTR17:[0-9]+]]
+; IS__TUNIT____-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* noalias nocapture nofree writeonly [[PTR1]], i8* noalias nocapture nofree readonly [[PTR2]], i32 noundef 8, i1 noundef true) #[[ATTR18:[0-9]+]]
 ; IS__TUNIT____-NEXT:    ret i32 4
 ;
-; IS__CGSCC____: Function Attrs: argmemonly nofree nosync nounwind willreturn
+; IS__CGSCC____: Function Attrs: argmemonly nofree nounwind willreturn
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@memcpy_volatile
 ; IS__CGSCC____-SAME: (i8* nocapture nofree writeonly [[PTR1:%.*]], i8* nocapture nofree readonly [[PTR2:%.*]]) #[[ATTR10:[0-9]+]] {
-; IS__CGSCC____-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* noalias nocapture nofree writeonly [[PTR1]], i8* noalias nocapture nofree readonly [[PTR2]], i32 noundef 8, i1 noundef true) #[[ATTR18:[0-9]+]]
+; IS__CGSCC____-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* noalias nocapture nofree writeonly [[PTR1]], i8* noalias nocapture nofree readonly [[PTR2]], i32 noundef 8, i1 noundef true) #[[ATTR19:[0-9]+]]
 ; IS__CGSCC____-NEXT:    ret i32 4
 ;
   call void @llvm.memcpy(i8* %ptr1, i8* %ptr2, i32 8, i1 1)
   ret i32 4
 }
 
 ; TEST 15 - positive, non-volatile intrinsic.
 
 ; It is odd to add nocapture but a result of the llvm.memset nocapture.
 ;
 define i32 @memset_non_volatile(i8* %ptr1, i8 %val) {
 ; IS__TUNIT____: Function Attrs: argmemonly nofree nosync nounwind willreturn writeonly
 ; IS__TUNIT____-LABEL: define {{[^@]+}}@memset_non_volatile
 ; IS__TUNIT____-SAME: (i8* nocapture nofree writeonly [[PTR1:%.*]], i8 [[VAL:%.*]]) #[[ATTR11:[0-9]+]] {
-; IS__TUNIT____-NEXT:    call void @llvm.memset.p0i8.i32(i8* nocapture nofree writeonly [[PTR1]], i8 [[VAL]], i32 noundef 8, i1 noundef false) #[[ATTR18:[0-9]+]]
+; IS__TUNIT____-NEXT:    call void @llvm.memset.p0i8.i32(i8* nocapture nofree writeonly [[PTR1]], i8 [[VAL]], i32 noundef 8, i1 noundef false) #[[ATTR19:[0-9]+]]
 ; IS__TUNIT____-NEXT:    ret i32 4
 ;
 ; IS__CGSCC____: Function Attrs: argmemonly nofree nosync nounwind willreturn writeonly
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@memset_non_volatile
 ; IS__CGSCC____-SAME: (i8* nocapture nofree writeonly [[PTR1:%.*]], i8 [[VAL:%.*]]) #[[ATTR11:[0-9]+]] {
-; IS__CGSCC____-NEXT:    call void @llvm.memset.p0i8.i32(i8* nocapture nofree writeonly [[PTR1]], i8 [[VAL]], i32 noundef 8, i1 noundef false) #[[ATTR19:[0-9]+]]
+; IS__CGSCC____-NEXT:    call void @llvm.memset.p0i8.i32(i8* nocapture nofree writeonly [[PTR1]], i8 [[VAL]], i32 noundef 8, i1 noundef false) #[[ATTR20:[0-9]+]]
 ; IS__CGSCC____-NEXT:    ret i32 4
 ;
   call void @llvm.memset(i8* %ptr1, i8 %val, i32 8, i1 0)
   ret i32 4
 }
 
 ; TEST 16 - negative, inline assembly.
 
;
   call float @llvm.cos(float %x)
   ret i32 4
 }
 
 define float @cos_test2(float %x) {
 ; IS__TUNIT____: Function Attrs: nofree nosync nounwind readnone willreturn
 ; IS__TUNIT____-LABEL: define {{[^@]+}}@cos_test2
 ; IS__TUNIT____-SAME: (float [[X:%.*]]) #[[ATTR15]] {
-; IS__TUNIT____-NEXT:    [[C:%.*]] = call float @llvm.cos.f32(float [[X]]) #[[ATTR19:[0-9]+]]
+; IS__TUNIT____-NEXT:    [[C:%.*]] = call float @llvm.cos.f32(float [[X]]) #[[ATTR20:[0-9]+]]
 ; IS__TUNIT____-NEXT:    ret float [[C]]
 ;
 ; IS__CGSCC____: Function Attrs: nofree nosync nounwind readnone willreturn
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@cos_test2
 ; IS__CGSCC____-SAME: (float [[X:%.*]]) #[[ATTR16:[0-9]+]] {
-; IS__CGSCC____-NEXT:    [[C:%.*]] = call float @llvm.cos.f32(float [[X]]) #[[ATTR20:[0-9]+]]
+; IS__CGSCC____-NEXT:    [[C:%.*]] = call float @llvm.cos.f32(float [[X]]) #[[ATTR21:[0-9]+]]
 ; IS__CGSCC____-NEXT:    ret float [[C]]
 ;
   %c = call float @llvm.cos(float %x)
   ret float %c
 }
 ;.
 ; IS__TUNIT____: attributes #[[ATTR0]] = { nofree nosync nounwind optsize readnone ssp uwtable willreturn }
 ; IS__TUNIT____: attributes #[[ATTR1]] = { argmemonly nofree norecurse nosync nounwind uwtable willreturn }
 ; IS__TUNIT____: attributes #[[ATTR2]] = { argmemonly nofree norecurse nounwind uwtable willreturn }
 ; IS__TUNIT____: attributes #[[ATTR3]] = { noinline nosync nounwind uwtable }
 ; IS__TUNIT____: attributes #[[ATTR4]] = { noinline nounwind uwtable }
 ; IS__TUNIT____: attributes #[[ATTR5]] = { nofree noinline noreturn nosync nounwind readnone uwtable willreturn }
 ; IS__TUNIT____: attributes #[[ATTR6]] = { nofree nounwind willreturn }
 ; IS__TUNIT____: attributes #[[ATTR7]] = { nofree nounwind }
 ; IS__TUNIT____: attributes #[[ATTR8]] = { nofree nosync nounwind willreturn }
 ; IS__TUNIT____: attributes #[[ATTR9]] = { nofree nosync nounwind }
-; IS__TUNIT____: attributes #[[ATTR10]] = { argmemonly nofree nosync nounwind willreturn }
+; IS__TUNIT____: attributes #[[ATTR10]] = { argmemonly nofree nounwind willreturn }
 ; IS__TUNIT____: attributes #[[ATTR11]] = { argmemonly nofree nosync nounwind willreturn writeonly }
 ; IS__TUNIT____: attributes #[[ATTR12:[0-9]+]] = { convergent readnone }
 ; IS__TUNIT____: attributes #[[ATTR13]] = { readnone }
 ; IS__TUNIT____: attributes #[[ATTR14]] = { nounwind }
 ; IS__TUNIT____: attributes #[[ATTR15]] = { nofree nosync nounwind readnone willreturn }
-; IS__TUNIT____: attributes #[[ATTR16:[0-9]+]] = { nofree nosync nounwind readnone speculatable willreturn }
-; IS__TUNIT____: attributes #[[ATTR17]] = { willreturn }
-; IS__TUNIT____: attributes #[[ATTR18]] = { willreturn writeonly }
-; IS__TUNIT____: attributes #[[ATTR19]] = { readnone willreturn }
+; IS__TUNIT____: attributes #[[ATTR16:[0-9]+]] = { argmemonly nofree nosync nounwind willreturn }
+; IS__TUNIT____: attributes #[[ATTR17:[0-9]+]] = { nofree nosync nounwind readnone speculatable willreturn }
+; IS__TUNIT____: attributes #[[ATTR18]] = { willreturn }
+; IS__TUNIT____: attributes #[[ATTR19]] = { willreturn writeonly }
+; IS__TUNIT____: attributes #[[ATTR20]] = { readnone willreturn }
 ;.
 ; IS__CGSCC_OPM: attributes #[[ATTR0]] = { nofree norecurse nosync nounwind optsize readnone ssp uwtable willreturn }
 ; IS__CGSCC_OPM: attributes #[[ATTR1]] = { argmemonly nofree norecurse nosync nounwind uwtable willreturn }
 ; IS__CGSCC_OPM: attributes #[[ATTR2]] = { argmemonly nofree norecurse nounwind uwtable willreturn }
 ; IS__CGSCC_OPM: attributes #[[ATTR3]] = { noinline nosync nounwind uwtable }
 ; IS__CGSCC_OPM: attributes #[[ATTR4]] = { noinline nounwind uwtable }
 ; IS__CGSCC_OPM: attributes #[[ATTR5]] = { nofree noinline noreturn nosync nounwind readnone uwtable willreturn }
 ; IS__CGSCC_OPM: attributes #[[ATTR6]] = { nofree norecurse nounwind willreturn }
 ; IS__CGSCC_OPM: attributes #[[ATTR7]] = { nofree norecurse nounwind }
 ; IS__CGSCC_OPM: attributes #[[ATTR8]] = { nofree norecurse nosync nounwind willreturn }
 ; IS__CGSCC_OPM: attributes #[[ATTR9]] = { nofree norecurse nosync nounwind }
-; IS__CGSCC_OPM: attributes #[[ATTR10]] = { argmemonly nofree nosync nounwind willreturn }
+; IS__CGSCC_OPM: attributes #[[ATTR10]] = { argmemonly nofree nounwind willreturn }
 ; IS__CGSCC_OPM: attributes #[[ATTR11]] = { argmemonly nofree nosync nounwind willreturn writeonly }
 ; IS__CGSCC_OPM: attributes #[[ATTR12:[0-9]+]] = { convergent readnone }
 ; IS__CGSCC_OPM: attributes #[[ATTR13]] = { readnone }
 ; IS__CGSCC_OPM: attributes #[[ATTR14]] = { nounwind }
 ; IS__CGSCC_OPM: attributes #[[ATTR15]] = { nofree norecurse nosync nounwind readnone willreturn }
 ; IS__CGSCC_OPM: attributes #[[ATTR16]] = { nofree nosync nounwind readnone willreturn }
-; IS__CGSCC_OPM: attributes #[[ATTR17:[0-9]+]] = { nofree nosync nounwind readnone speculatable willreturn }
-; IS__CGSCC_OPM: attributes #[[ATTR18]] = { willreturn }
-; IS__CGSCC_OPM: attributes #[[ATTR19]] = { willreturn writeonly }
-; IS__CGSCC_OPM: attributes #[[ATTR20]] = { readnone willreturn }
+; IS__CGSCC_OPM: attributes #[[ATTR17:[0-9]+]] = { argmemonly nofree nosync nounwind willreturn }
+; IS__CGSCC_OPM: attributes #[[ATTR18:[0-9]+]] = { nofree nosync nounwind readnone speculatable willreturn }
+; IS__CGSCC_OPM: attributes #[[ATTR19]] = { willreturn }
+; IS__CGSCC_OPM: attributes #[[ATTR20]] = { willreturn writeonly }
+; IS__CGSCC_OPM: attributes #[[ATTR21]] = { readnone willreturn }
 ;.
 ; IS__CGSCC_NPM: attributes #[[ATTR0]] = { nofree norecurse nosync nounwind optsize readnone ssp uwtable willreturn }
 ; IS__CGSCC_NPM: attributes #[[ATTR1]] = { argmemonly nofree norecurse nosync nounwind uwtable willreturn }
 ; IS__CGSCC_NPM: attributes #[[ATTR2]] = { argmemonly nofree norecurse nounwind uwtable willreturn }
 ; IS__CGSCC_NPM: attributes #[[ATTR3]] = { noinline nosync nounwind uwtable }
 ; IS__CGSCC_NPM: attributes #[[ATTR4]] = { noinline nounwind uwtable }
 ; IS__CGSCC_NPM: attributes #[[ATTR5]] = { nofree noinline norecurse noreturn nosync nounwind readnone uwtable willreturn }
 ; IS__CGSCC_NPM: attributes #[[ATTR6]] = { nofree norecurse nounwind willreturn }
 ; IS__CGSCC_NPM: attributes #[[ATTR7]] = { nofree norecurse nounwind }
 ; IS__CGSCC_NPM: attributes #[[ATTR8]] = { nofree norecurse nosync nounwind willreturn }
 ; IS__CGSCC_NPM: attributes #[[ATTR9]] = { nofree norecurse nosync nounwind }
-; IS__CGSCC_NPM: attributes #[[ATTR10]] = { argmemonly nofree nosync nounwind willreturn }
+; IS__CGSCC_NPM: attributes #[[ATTR10]] = { argmemonly nofree nounwind willreturn }
 ; IS__CGSCC_NPM: attributes #[[ATTR11]] = { argmemonly nofree nosync nounwind willreturn writeonly }
 ; IS__CGSCC_NPM: attributes #[[ATTR12:[0-9]+]] = { convergent readnone }
 ; IS__CGSCC_NPM: attributes #[[ATTR13]] = { readnone }
 ; IS__CGSCC_NPM: attributes #[[ATTR14]] = { nounwind }
 ; IS__CGSCC_NPM: attributes #[[ATTR15]] = { nofree norecurse nosync nounwind readnone willreturn }
 ; IS__CGSCC_NPM: attributes #[[ATTR16]] = { nofree nosync nounwind readnone willreturn }
-; IS__CGSCC_NPM: attributes #[[ATTR17:[0-9]+]] = { nofree nosync nounwind readnone speculatable willreturn }
-; IS__CGSCC_NPM: attributes #[[ATTR18]] = { willreturn }
-; IS__CGSCC_NPM: attributes #[[ATTR19]] = { willreturn writeonly }
-; IS__CGSCC_NPM: attributes #[[ATTR20]] = { readnone willreturn }
+; IS__CGSCC_NPM: attributes #[[ATTR17:[0-9]+]] = { argmemonly nofree nosync nounwind willreturn }
+; IS__CGSCC_NPM: attributes #[[ATTR18:[0-9]+]] = { nofree nosync nounwind readnone speculatable willreturn }
+; IS__CGSCC_NPM: attributes #[[ATTR19]] = { willreturn }
+; IS__CGSCC_NPM: attributes #[[ATTR20]] = { willreturn writeonly }
+; IS__CGSCC_NPM: attributes #[[ATTR21]] = { readnone willreturn }
 ;.