[rs4gc] Strip memory related attributes consistently

[reames]

Summary

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:

1. 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.
2. 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.

I'm mostly posting for review to give a chance for a sanity check, and a perf test if interested. I could see this negatively impacting optimization after lowering. I doubt it's a large effect, but that might be worth measuring.

[skatkov]

Hi Philip, it seems this change causes functional failure.

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

2635

It seems that leads to functional issue.
The problem here is that this function is invoked for any declared function in a module including llvm intrinsics which are not covered by statepoint instruction.

In my failed test it appeared llvm.x86.sse2.cvttsd2si64 and as a result readnone attribute has been cleared from it.

Later it could not find this intrinsic while linking:
Cannot select: intrinsic %llvm.x86.sse2.cvttsd2si64

[reames]

I landed a change (01801d5274) which should address the reported issue. (It was essentially a latent bug in the existing code.)

Once that bakes for a bit, I'd appreciate a retest cycle to see if anything else falls out. That one definitely wasn't anything I was expecting.

[reames]

In D99802#2699527, @reames wrote:

Once that bakes for a bit, I'd appreciate a retest cycle to see if anything else falls out. That one definitely wasn't anything I was expecting.

@skatkov Any update on this retest cycle?

[skatkov]

In D99802#2739846, @reames wrote:

In D99802#2699527, @reames wrote:

Once that bakes for a bit, I'd appreciate a retest cycle to see if anything else falls out. That one definitely wasn't anything I was expecting.

@skatkov Any update on this retest cycle?

Hi Philip, sorry for the delay, testing is in progress. I'm hoping the result will be available by the end of this week.

[skatkov]

Sorry for the long delay.

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

normalizeForInvokeSafepoint(BasicBlock *BB, BasicBlock *InvokeParent,
   assert(!isa<PHINode>(Ret->begin()) &&
          "All PHI nodes should have been removed!");
 
   // At this point, we can safely insert a gc.relocate or gc.result as the first
   // instruction in Ret if needed.
   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,
                                             AttributeList AL) {
   if (AL.isEmpty())
     return AL;
 
   // 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()) {
     if (isStatepointDirectiveAttr(A))
       FnAttrs.remove(A);
   }
 
   // Just skip parameter and return attributes for now
static void RemoveNonValidAttrAtIndex(LLVMContext &Ctx, AttrHolder &AH,
                                       unsigned Index) {
   AttrBuilder R;
   if (AH.getDereferenceableBytes(Index))
     R.addAttribute(Attribute::get(Ctx, Attribute::Dereferenceable,
                                   AH.getDereferenceableBytes(Index)));
   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);
 
   if (!R.empty())
     AH.setAttributes(AH.getAttributes().removeAttributes(Ctx, Index, R));
 }
 
 static void stripNonValidAttributesFromPrototype(Function &F) {
static void stripNonValidAttributesFromPrototype(Function &F) {
   for (Argument &A : F.args())
     if (isa<PointerType>(A.getType()))
       RemoveNonValidAttrAtIndex(Ctx, F,
                                 A.getArgNo() + AttributeList::FirstArgIndex);
 
   if (isa<PointerType>(F.getReturnType()))
     RemoveNonValidAttrAtIndex(Ctx, F, AttributeList::ReturnIndex);
 
-  for (auto Attr : {Attribute::NoSync, Attribute::NoFree})
+  for (auto Attr : FnAttrsToStrip)
     F.removeFnAttr(Attr);
 }
 
 /// Certain metadata on instructions are invalid after running RS4GC.
 /// Optimizations that run after RS4GC can incorrectly use this metadata to
 /// optimize functions. We drop such metadata on the instruction.
 static void stripInvalidMetadataFromInstruction(Instruction &I) {
   if (!isa<LoadInst>(I) && !isa<StoreInst>(I))

llvm/test/Transforms/RewriteStatepointsForGC/strip-invalid-attributes.ll

 }
 
 define i8 addrspace(1)* @nosync(i8 addrspace(1)* %arg) nosync gc "statepoint-example" {
 ; CHECK: define i8 addrspace(1)* @nosync(i8 addrspace(1)* %arg) gc "statepoint-example" {
   call void @f()
   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
+}
+