[llvm] 3f1c218 - [rs4gc] Strip memory related attributes consistently

[Philip Reames via llvm-commits]

Author: Philip Reames
Date: 2021-05-14T07:54:56-07:00
New Revision: 3f1c218318ed2e4f37d169f1bfbb39be657dc0b4

URL: https://github.com/llvm/llvm-project/commit/3f1c218318ed2e4f37d169f1bfbb39be657dc0b4
DIFF: https://github.com/llvm/llvm-project/commit/3f1c218318ed2e4f37d169f1bfbb39be657dc0b4.diff

LOG: [rs4gc] Strip memory related attributes consistently

I noticed that rs4gc is not stripping a number of memory aliasing related attributes. We do strip some from call sites, but don't strip the same ones from declarations or parameters.

Why do we need to strip these? Two answers:

    Safepoints conceptually read and write to the entire garbage collected heap in the physical model. We need this to preserve ordering of all loads and stores with respect to possible relocation.
    We can infer other attributes from these. For instance, readnone can imply both nofree and nosync. Both of which don't hold after physical rewriting.

Note: This exposed a latent issue which was fixed a couple weeks back in 01801d5274.

Differential Revision: https://reviews.llvm.org/D99802

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
    llvm/test/Transforms/RewriteStatepointsForGC/strip-invalid-attributes.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
index 369fa8eca65c5..051c6f976e527 100644
--- a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
+++ b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
@@ -1333,6 +1333,27 @@ normalizeForInvokeSafepoint(BasicBlock *BB, BasicBlock *InvokeParent,
   return Ret;
 }
 
+// List of all function attributes which must be stripped when lowering from
+// abstract machine model to physical machine model.  Essentially, these are
+// all the effects a safepoint might have which we ignored in the abstract
+// machine model for purposes of optimization.  We have to strip these on
+// both function declarations and call sites.
+static constexpr Attribute::AttrKind FnAttrsToStrip[] =
+  {Attribute::ReadNone, Attribute::ReadOnly, Attribute::WriteOnly,
+   Attribute::ArgMemOnly, Attribute::InaccessibleMemOnly,
+   Attribute::InaccessibleMemOrArgMemOnly,
+   Attribute::NoSync, Attribute::NoFree};
+
+// List of all parameter and return attributes which must be stripped when
+// lowering from the abstract machine model.  Note that we list attributes
+// here which aren't valid as return attributes, that is okay.  There are
+// also some additional attributes with arguments which are handled
+// explicitly and are not in this list.
+static constexpr Attribute::AttrKind ParamAttrsToStrip[] =
+  {Attribute::ReadNone, Attribute::ReadOnly, Attribute::WriteOnly,
+   Attribute::NoAlias, Attribute::NoFree};
+
+
 // Create new attribute set containing only attributes which can be transferred
 // from original call to the safepoint.
 static AttributeList legalizeCallAttributes(LLVMContext &Ctx,
@@ -1342,8 +1363,7 @@ static AttributeList legalizeCallAttributes(LLVMContext &Ctx,
 
   // Remove the readonly, readnone, and statepoint function attributes.
   AttrBuilder FnAttrs = AL.getFnAttributes();
-  for (auto Attr : {Attribute::ReadNone, Attribute::ReadOnly,
-                    Attribute::NoSync, Attribute::NoFree})
+  for (auto Attr : FnAttrsToStrip)
     FnAttrs.removeAttribute(Attr);
 
   for (Attribute A : AL.getFnAttributes()) {
@@ -2583,7 +2603,7 @@ static void RemoveNonValidAttrAtIndex(LLVMContext &Ctx, AttrHolder &AH,
   if (AH.getDereferenceableOrNullBytes(Index))
     R.addAttribute(Attribute::get(Ctx, Attribute::DereferenceableOrNull,
                                   AH.getDereferenceableOrNullBytes(Index)));
-  for (auto Attr : {Attribute::NoAlias, Attribute::NoFree})
+  for (auto Attr : ParamAttrsToStrip)
     if (AH.getAttributes().hasAttribute(Index, Attr))
       R.addAttribute(Attr);
 
@@ -2612,7 +2632,7 @@ static void stripNonValidAttributesFromPrototype(Function &F) {
   if (isa<PointerType>(F.getReturnType()))
     RemoveNonValidAttrAtIndex(Ctx, F, AttributeList::ReturnIndex);
 
-  for (auto Attr : {Attribute::NoSync, Attribute::NoFree})
+  for (auto Attr : FnAttrsToStrip)
     F.removeFnAttr(Attr);
 }
 

diff  --git a/llvm/test/Transforms/RewriteStatepointsForGC/strip-invalid-attributes.ll b/llvm/test/Transforms/RewriteStatepointsForGC/strip-invalid-attributes.ll
index 5e551293977c4..f448b5b8dff6f 100644
--- a/llvm/test/Transforms/RewriteStatepointsForGC/strip-invalid-attributes.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/strip-invalid-attributes.ll
@@ -55,3 +55,39 @@ define i8 addrspace(1)* @nosync(i8 addrspace(1)* %arg) nosync gc "statepoint-exa
   ret i8 addrspace(1)* %arg
 }
 
+define i8 addrspace(1)* @readnone(i8 addrspace(1)* readnone %arg) readnone gc "statepoint-example" {
+; CHECK: define i8 addrspace(1)* @readnone(i8 addrspace(1)* %arg) gc "statepoint-example" {
+  call void @f()
+  ret i8 addrspace(1)* %arg
+}
+
+define i8 addrspace(1)* @readonly(i8 addrspace(1)* readonly %arg) readonly gc "statepoint-example" {
+; CHECK: define i8 addrspace(1)* @readonly(i8 addrspace(1)* %arg) gc "statepoint-example" {
+  call void @f()
+  ret i8 addrspace(1)* %arg
+}
+
+define i8 addrspace(1)* @writeonly(i8 addrspace(1)* writeonly %arg) writeonly gc "statepoint-example" {
+; CHECK: define i8 addrspace(1)* @writeonly(i8 addrspace(1)* %arg) gc "statepoint-example" {
+  call void @f()
+  ret i8 addrspace(1)* %arg
+}
+
+define i8 addrspace(1)* @argmemonly(i8 addrspace(1)* %arg) argmemonly gc "statepoint-example" {
+; CHECK: define i8 addrspace(1)* @argmemonly(i8 addrspace(1)* %arg) gc "statepoint-example" {
+  call void @f()
+  ret i8 addrspace(1)* %arg
+}
+
+define i8 addrspace(1)* @inaccessiblememonly(i8 addrspace(1)* %arg) inaccessiblememonly gc "statepoint-example" {
+; CHECK: define i8 addrspace(1)* @inaccessiblememonly(i8 addrspace(1)* %arg) gc "statepoint-example" {
+  call void @f()
+  ret i8 addrspace(1)* %arg
+}
+
+define i8 addrspace(1)* @inaccessiblemem_or_argmemonly(i8 addrspace(1)* %arg) inaccessiblemem_or_argmemonly gc "statepoint-example" {
+; CHECK: define i8 addrspace(1)* @inaccessiblemem_or_argmemonly(i8 addrspace(1)* %arg) gc "statepoint-example" {
+  call void @f()
+  ret i8 addrspace(1)* %arg
+}
+