IR: Reject unsized sret in verifier

[arsenm]

Summary

The same is enforced for the other ABI pointer attributes.

Not sure why MemCpyOpt/memcpy.ll seems to have been testing for
exactly this with a referenced radar number, although I don't see how
that makes sense.

[jdoerfert]

I'm in favor.

llvm/test/Transforms/MemCpyOpt/memcpy.ll

234

FWIW, this was introduced in bd254f260127 and I cannot tell if such code should even be legal in the first place (and this patch says its not)

[rjmccall]

sret marks that a parameter needs the special indirect-return argument ABI, which does *not* require a sized type on many architectures. Swift actually relies on being able to trigger this ABI on a value of unknown size. Swift can, of course, just use a meaningless sized type, but since I assume you intend to actually use the type for something, I am concerned.

[arsenm]

In D88325#2301253, @rjmccall wrote:

sret marks that a parameter needs the special indirect-return argument ABI, which does *not* require a sized type on many architectures. Swift actually relies on being able to trigger this ABI on a value of unknown size. Swift can, of course, just use a meaningless sized type, but since I assume you intend to actually use the type for something, I am concerned.

Well, I'm concerned this edge case apparently exists and has very little test coverage anywhere. It's the odd one out among the other type carrying attributes and I dislike the asymmetry with byval. Is there any real reason to use an unsized type over a 0 sized type here?

[arsenm]

Rebase

[rjmccall]

Inconsistency between things that are different is expected. byval intrinsically requires a sized type because the compiler has to know how large of an object to allocate + copy in the argument area. sret does not require a type on any target I know of, and if there are places that expect the type to be meaningful, they are almost certainly buggy in the presence of dynamically-sized types. While C has only very restricted support for dynamically-sized types, I believe Fortran's support is broader, and so psABIs usually nod at them in various ways — it's not something that's novel to Swift.

[efriedma]

What do we use the sret type for, anyway? I tried looking around, but the only actual use I can find is Value::getPointerAlignment. (SelectionDAG stores the type, but doesn't actually use it for anything, as far as I can tell. And I'm not seeing any other usage.) We should probably consider just dropping the type from sret.

[arsenm]

In D88325#3931206, @efriedma wrote:

What do we use the sret type for, anyway? I tried looking around, but the only actual use I can find is Value::getPointerAlignment. (SelectionDAG stores the type, but doesn't actually use it for anything, as far as I can tell. And I'm not seeing any other usage.) We should probably consider just dropping the type from sret.

objectsize looks at the allocated size at least.

[rjmccall]

If the type is just used for optimization purposes, we have other attributes that work more generally for pointer-typed arguments.

llvm/lib/IR/Value.cpp

uint64_t Value::getPointerDereferenceableBytes(const DataLayout &DL,
 
   uint64_t DerefBytes = 0;
   CanBeNull = false;
   if (const Argument *A = dyn_cast<Argument>(this)) {
     DerefBytes = A->getDereferenceableBytes();
     if (DerefBytes == 0) {
       // Handle byval/byref/inalloca/preallocated arguments
       if (Type *ArgMemTy = A->getPointeeInMemoryValueType()) {
-        if (ArgMemTy->isSized()) {
-          // FIXME: Why isn't this the type alloc size?
-          DerefBytes = DL.getTypeStoreSize(ArgMemTy).getKnownMinSize();
-        }
-      }
-    }
 
     if (DerefBytes == 0) {
       DerefBytes = A->getDereferenceableOrNullBytes();
       CanBeNull = true;
     }
   } else if (const auto *Call = dyn_cast<CallBase>(this)) {
     DerefBytes = Call->getDereferenceableBytes(AttributeList::ReturnIndex);
     if (DerefBytes == 0) {
if (!Alignment) {
       }
     }
     return Alignment.valueOrOne();
   } else if (const Argument *A = dyn_cast<Argument>(this)) {
     const MaybeAlign Alignment = A->getParamAlign();
     if (!Alignment && A->hasStructRetAttr()) {
       // An sret parameter has at least the ABI alignment of the return type.
       Type *EltTy = A->getParamStructRetType();
-      if (EltTy->isSized())
-        return DL.getABITypeAlign(EltTy);
     }
     return Alignment.valueOrOne();
   } else if (const AllocaInst *AI = dyn_cast<AllocaInst>(this)) {
     return AI->getAlign();
   } else if (const auto *Call = dyn_cast<CallBase>(this)) {
     MaybeAlign Alignment = Call->getRetAlign();
     if (!Alignment && Call->getCalledFunction())
       Alignment = Call->getCalledFunction()->getAttributes().getRetAlignment();

llvm/lib/IR/Verifier.cpp

Assert(!AttrBuilder(Attrs).overlaps(IncompatibleAttrs),
          "Wrong types for attribute: " +
              AttributeSet::get(Context, IncompatibleAttrs).getAsString(),
          V);
 
   if (PointerType *PTy = dyn_cast<PointerType>(Ty)) {
     SmallPtrSet<Type*, 4> Visited;
     if (!PTy->getElementType()->isSized(&Visited)) {
       Assert(!Attrs.hasAttribute(Attribute::ByVal) &&
+             !Attrs.hasAttribute(Attribute::StructRet) &&
              !Attrs.hasAttribute(Attribute::ByRef) &&
              !Attrs.hasAttribute(Attribute::InAlloca) &&
              !Attrs.hasAttribute(Attribute::Preallocated),
-             "Attributes 'byval', 'byref', 'inalloca', and 'preallocated' do not "
+             "Attributes 'byval', 'sret', 'byref', 'inalloca', and 'preallocated' do not "
              "support unsized types!",
              V);
     }
     if (!isa<PointerType>(PTy->getElementType()))
       Assert(!Attrs.hasAttribute(Attribute::SwiftError),
              "Attribute 'swifterror' only applies to parameters "
              "with pointer to pointer type!",
              V);

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

if (AllocaInst *A = dyn_cast<AllocaInst>(cpyDest)) {
 
     if (A->getDereferenceableBytes() < srcSize) {
       // If the destination is an sret parameter then only accesses that are
       // outside of the returned struct type can trap.
       if (!A->hasStructRetAttr())
         return false;
 
       Type *StructTy = A->getParamStructRetType();
-      if (!StructTy->isSized()) {
-        // The call may never return and hence the copy-instruction may never
-        // be executed, and therefore it's not safe to say "the destination
-        // has at least <cpyLen> bytes, as implied by the copy-instruction",
-        return false;
-      }
-
       uint64_t destSize = DL.getTypeAllocSize(StructTy);
       if (destSize < srcSize)
         return false;
     }
   } else {
     return false;
   }
 

llvm/test/Transforms/MemCpyOpt/memcpy.ll

; CHECK: f2
   call void @f1(%struct.big* sret %tmp)
   %0 = bitcast %struct.big* %b to i8*
   %1 = bitcast %struct.big* %tmp to i8*
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %0, i8* align 4 %1, i64 200, i1 false)
   call void @f2(%struct.big* %b)
   ret void
 }
 
-; rdar://14073661.
-; Test10 triggered assertion when the compiler try to get the size of the
-; opaque type of *x, where the x is the formal argument with attribute 'sret'.
-
-%opaque = type opaque
-declare void @foo(i32* noalias nocapture)
-
-define void @test10(%opaque* noalias nocapture sret %x, i32 %y) {
-  %a = alloca i32, align 4
-  store i32 %y, i32* %a
-  call void @foo(i32* noalias nocapture %a)
-  %c = load i32, i32* %a
-  %d = bitcast %opaque* %x to i32*
-  store i32 %c, i32* %d
-  ret void
-}
-
 ; don't create new addressspacecasts when we don't know they're safe for the target
 define void @test11([20 x i32] addrspace(1)* nocapture dereferenceable(80) %P) {
   %A = alloca [20 x i32], align 4
   %a = bitcast [20 x i32]* %A to i8*
   %b = bitcast [20 x i32] addrspace(1)* %P to i8 addrspace(1)*
   call void @llvm.memset.p0i8.i64(i8* align 4 %a, i8 0, i64 80, i1 false)
   call void @llvm.memcpy.p1i8.p0i8.i64(i8 addrspace(1)* align 4 %b, i8* align 4 %a, i64 80, i1 false)
   ret void

llvm/test/Transforms/MergeFunc/apply_function_attributes.ll

 ; RUN: opt -S -mergefunc < %s | FileCheck %s
 
-%Opaque_type = type opaque
+%Empty_type = type {}
 %S2i = type <{ i64, i64 }>
 %D2i = type <{ i64, i64 }>
 %Di = type <{ i32 }>
 %Si = type <{ i32 }>
 
-define void @B(%Opaque_type* sret %a, %S2i* %b, i32* %xp, i32* %yp) {
+define void @B(%Empty_type* sret %a, %S2i* %b, i32* %xp, i32* %yp) {
   %x = load i32, i32* %xp
   %y = load i32, i32* %yp
   %sum = add i32 %x, %y
   %sum2 = add i32 %sum, %y
   %sum3 = add i32 %sum2, %y
   ret void
 }
 
-define void @C(%Opaque_type* sret %a, %S2i* %b, i32* %xp, i32* %yp) {
+define void @C(%Empty_type* sret %a, %S2i* %b, i32* %xp, i32* %yp) {
   %x = load i32, i32* %xp
   %y = load i32, i32* %yp
   %sum = add i32 %x, %y
   %sum2 = add i32 %sum, %y
   %sum3 = add i32 %sum2, %y
   ret void
 }
 
-define void @A(%Opaque_type* sret %a, %D2i* %b, i32* %xp, i32* %yp) {
+define void @A(%Empty_type* sret %a, %D2i* %b, i32* %xp, i32* %yp) {
   %x = load i32, i32* %xp
   %y = load i32, i32* %yp
   %sum = add i32 %x, %y
   %sum2 = add i32 %sum, %y
   %sum3 = add i32 %sum2, %y
   ret void
 }
 
 ; Make sure we transfer the parameter attributes to the call site.
-; CHECK-LABEL: define void @C(%Opaque_type* sret
-; CHECK:  tail call void bitcast (void (%Opaque_type*, %D2i*, i32*, i32*)* @A to void (%Opaque_type*, %S2i*, i32*, i32*)*)(%Opaque_type* sret %0, %S2i* %1, i32* %2, i32* %3)
+; CHECK-LABEL: define void @C(%Empty_type* sret
+; CHECK:  tail call void bitcast (void (%Empty_type*, %D2i*, i32*, i32*)* @A to void (%Empty_type*, %S2i*, i32*, i32*)*)(%Empty_type* sret %0, %S2i* %1, i32* %2, i32* %3)
 ; CHECK:  ret void
 
 
 ; Make sure we transfer the parameter attributes to the call site.
-; CHECK-LABEL: define void @B(%Opaque_type* sret
+; CHECK-LABEL: define void @B(%Empty_type* sret
 ; CHECK:  %5 = bitcast
-; CHECK:  tail call void @A(%Opaque_type* sret %0, %D2i* %5, i32* %2, i32* %3)
+; CHECK:  tail call void @A(%Empty_type* sret %0, %D2i* %5, i32* %2, i32* %3)
 ; CHECK:  ret void
 

llvm/test/Verifier/byref.ll

 ; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
 
 ; CHECK: Attribute 'byref' type does not match parameter!
 ; CHECK-NEXT: void (i32*)* @byref_mismatched_pointee_type0
 define void @byref_mismatched_pointee_type0(i32* byref(i8)) {
   ret void
 }
 
 ; CHECK: Attribute 'byref' type does not match parameter!
 ; CHECK-NEXT: void (i8*)* @byref_mismatched_pointee_type1
 define void @byref_mismatched_pointee_type1(i8* byref(i32)) {
   ret void
 }
 
 %opaque.ty = type opaque
 
-; CHECK: Attributes 'byval', 'byref', 'inalloca', and 'preallocated' do not support unsized types!
+; CHECK: Attributes 'byval', 'sret', 'byref', 'inalloca', and 'preallocated' do not support unsized types!
 ; CHECK-NEXT: void (%opaque.ty*)* @byref_unsized
 define void @byref_unsized(%opaque.ty* byref(%opaque.ty)) {
   ret void
 }
 
 ; CHECK: Attributes 'byval', 'inalloca', 'preallocated', 'inreg', 'nest', 'byref', and 'sret' are incompatible!
 ; CHECK-NEXT: void (i32*)* @byref_byval
 define void @byref_byval(i32* byref(i32) byval(i32)) {

llvm/test/Verifier/sret.ll

 ; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
 
 declare void @a(i32* sret %a, i32* sret %b)
 ; CHECK: Cannot have multiple 'sret' parameters!
 
 declare void @b(i32* %a, i32* %b, i32* sret %c)
 ; CHECK: Attribute 'sret' is not on first or second parameter!
+
+%opaque.ty = type opaque
+
+declare void @c(%opaque.ty* sret %a)
+; CHECK: Attributes 'byval', 'sret', 'byref', 'inalloca', and 'preallocated' do not support unsized types!