[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

1327

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 ↗

(On Diff #344621)

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 {
 
   /// See AbstractAttribute::updateImpl(...).
   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);
 };
 
 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) {
-  if (auto *MI = dyn_cast<MemIntrinsic>(I))
-    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 (isNoSyncIntrinsic(&I))
-        return true;
-
       const auto &NoSyncAA = A.getAAFor<AANoSync>(
           *this, IRPosition::callsite_function(*CB), DepClassTy::REQUIRED);
       return NoSyncAA.isAssumedNoSync();
     }
 
     if (!I.isVolatile() && !isNonRelaxedAtomic(&I))
       return true;
 

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;
 }