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

365

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/Analysis/MemoryBuiltins.cpp

return Overflow ? unknown()
                     : std::make_pair(align(Size, I.getAlign()), Zero);
   }
   return unknown();
 }
 
 SizeOffsetType ObjectSizeOffsetVisitor::visitArgument(Argument &A) {
   Type *MemoryTy = A.getPointeeInMemoryValueType();
   // No interprocedural analysis is done at the moment.
-  if (!MemoryTy|| !MemoryTy->isSized()) {
+  if (!MemoryTy) {
     ++ObjectVisitorArgument;
     return unknown();
   }
 
   APInt Size(IntTyBits, DL.getTypeAllocSize(MemoryTy));
   return std::make_pair(align(Size, A.getParamAlign()), Zero);
 }
 

llvm/lib/IR/Value.cpp

uint64_t Value::getPointerDereferenceableBytes(const DataLayout &DL,
   uint64_t DerefBytes = 0;
   CanBeNull = false;
   CanBeFreed = UseDerefAtPointSemantics && canBeFreed();
   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->getRetDereferenceableBytes();
     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

if (Attrs.hasAttribute(Attribute::ByVal)) {
         Align MaxAlign(ParamMaxAlignment);
         Check(AttrAlign <= MaxAlign,
               "Attribute 'align' exceed the max size 2^14", V);
       }
       SmallPtrSet<Type *, 4> Visited;
       Check(Attrs.getByValType()->isSized(&Visited),
             "Attribute 'byval' does not support unsized types!", V);
     }
+
+    if (Attrs.hasAttribute(Attribute::StructRet)) {
+      SmallPtrSet<Type *, 4> Visited;
+      Check(Attrs.getStructRetType()->isSized(&Visited),
+            "Attribute 'sret' does not support unsized types!", V);
+    }
+
     if (Attrs.hasAttribute(Attribute::ByRef)) {
       SmallPtrSet<Type *, 4> Visited;
       Check(Attrs.getByRefType()->isSized(&Visited),
             "Attribute 'byref' does not support unsized types!", V);
     }
     if (Attrs.hasAttribute(Attribute::InAlloca)) {
       SmallPtrSet<Type *, 4> Visited;
       Check(Attrs.getInAllocaType()->isSized(&Visited),

llvm/test/Transforms/InstCombine/object-size-opaque.ll

-; RUN: opt -passes=instcombine -S %s | FileCheck %s
-%opaque = type opaque
-
-; CHECK: call i64 @llvm.objectsize.i64
-define void @foo(ptr sret(%opaque) %in, ptr %sizeptr) {
-  %size = call i64 @llvm.objectsize.i64(ptr %in, i1 0, i1 0, i1 0)
-  store i64 %size, ptr %sizeptr
-  ret void
-}
-
-declare i64 @llvm.objectsize.i64(ptr, i1, i1, i1)

llvm/test/Transforms/MemCpyOpt/memcpy.ll

entry:
   call void @f2(ptr %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
+%empty = type {}
 declare void @foo(ptr noalias nocapture)
 
-define void @test10(ptr noalias nocapture sret(%opaque) %x, i32 %y) {
+define void @test10(ptr noalias nocapture sret(%empty) %x, i32 %y) {
 ; CHECK-LABEL: @test10(
 ; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
 ; CHECK-NEXT:    store i32 [[Y:%.*]], ptr [[A]], align 4
 ; CHECK-NEXT:    call void @foo(ptr noalias nocapture [[A]])
 ; CHECK-NEXT:    [[C:%.*]] = load i32, ptr [[A]], align 4
 ; CHECK-NEXT:    store i32 [[C]], ptr [[X:%.*]], align 4
 ; CHECK-NEXT:    ret void
 ;
   %a = alloca i32, align 4
   store i32 %y, ptr %a
   call void @foo(ptr noalias nocapture %a)
   %c = load i32, ptr %a
   store i32 %c, ptr %x
   ret void
 }
 
 ; don't create new addressspacecasts when we don't know they're safe for the target
 define void @test11(ptr addrspace(1) nocapture dereferenceable(80) %P) {
 ; CHECK-LABEL: @test11(
 ; CHECK-NEXT:    call void @llvm.memset.p1.i64(ptr addrspace(1) align 4 [[P:%.*]], i8 0, i64 80, i1 false)
 ; CHECK-NEXT:    ret void
 ;
   %A = alloca [20 x i32], align 4
   call void @llvm.memset.p0.i64(ptr align 4 %A, i8 0, i64 80, i1 false)
   call void @llvm.memcpy.p1.p0.i64(ptr addrspace(1) align 4 %P, ptr align 4 %A, i64 80, i1 false)
   ret void
 }
 
 declare void @f1(ptr nocapture sret(%struct.big))
 declare void @f2(ptr)

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(%Opaque_type) %a, %S2i* %b, i32* %xp, i32* %yp) {
+define void @B(%Empty_type* sret(%Empty_type) %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(%Opaque_type) %a, %S2i* %b, i32* %xp, i32* %yp) {
+define void @C(%Empty_type* sret(%Empty_type) %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(%Opaque_type) %a, %D2i* %b, i32* %xp, i32* %yp) {
+define void @A(%Empty_type* sret(%Empty_type) %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(%Opaque_type) %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(%Empty_type) %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(%Opaque_type) %0, %D2i* %5, i32* %2, i32* %3)
+; CHECK:  tail call void @A(%Empty_type* sret(%Empty_type) %0, %D2i* %5, i32* %2, i32* %3)
 ; CHECK:  ret void
 

llvm/test/Verifier/sret.ll

 ; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
 
 declare void @a(i32* sret(i32) %a, i32* sret(i32) %b)
 ; CHECK: Cannot have multiple 'sret' parameters!
 
 declare void @b(i32* %a, i32* %b, i32* sret(i32) %c)
 ; CHECK: Attribute 'sret' is not on first or second parameter!
 
 ; CHECK: Attribute 'sret(i32)' applied to incompatible type!
 ; CHECK-NEXT: void (i32)* @not_ptr
 declare void @not_ptr(i32 sret(i32) %x)
+
+%opaque.ty = type opaque
+
+declare void @c(%opaque.ty* sret(%opaque.ty) %a)
+; CHECK: Attribute 'sret' does not support unsized types!