Support literal structs in mangled intrinsics.

[pgavlin]

Summary

Literal structs are mangled as 'ls_' ( mangled fields) 's'. Mangling for structs is also changed to add a 's_' prefix and a 's' postfix to handle nested structs and struct names that may collide with other mangled types.

[JosephTremoulet]

Expand overloaded-intrinsic-name.ll test to include an overload using a literal struct type and execute the new codepath?

[pgavlin]

Expand overloaded-intrinsic-name.ll test to include an overload using a literal struct type and execute the new codepath?

Done.

[JosephTremoulet]

LGTM, thanks

[majnemer]

lib/IR/Function.cpp

492–493

Please consistently use braces.

[pgavlin]

Addressed review feedback.

[pgavlin]

Ping: I think I've addressed all feedback.

[pgavlin]

@reames/@sanjoy, do you think you'll have a chance to take a look at this soon? This is hampering LLILC's ability to use statepoints with the SysV AMD64 ABI.

[reames]

The new literal struct naming rules seem fine, but the change in non-literal structs is not. Unless you have a strong motivation for it, I'd prefer to see that portion of the change reverted.

lib/IR/Function.cpp

493

I'm not sure the change in serialization is acceptable here. This will silently break previously valid code. Is there a reason why the nesting scheme needs to apply to non-literal structs? I think you can leave the old code in place for non-literal structures and just add the nesting for the new case.

test/CodeGen/Generic/overloaded-intrinsic-name.ll

47

This is the change I'm concerned about.

[pgavlin]

lib/IR/Function.cpp

493

Consider the following:

%si64 = type { i64 }
%s = type { i64 }

; struct
define %si64* @test_struct(%si64* %v, %s* %w) gc "statepoint-example" {
       %tok = call i32 (i64, i32, %si64* ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_p0si64f(i64 0, i32 0, %si64* ()* @return_si64, i32 0, i32 0, i32 0, i32 0)
       ;%tok = call i32 (i64, i32, %s* (i64)*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_p0si64f(i64 0, i32 0, %s* (i64)* @return_s, i32 1, i32 0, i64 0, i32 0, i32 0)
       ret %si64* %v
}

declare %si64* @return_si64()
declare %s* @return_s(i64)

declare i32 @llvm.experimental.gc.statepoint.p0f_p0si64f(i64, i32, %si64* ()*, i32, i32, ...)
;declare i32 @llvm.experimental.gc.statepoint.p0f_p0si64f(i64, i32, %s* (i64)*, i32, i32, ...)

The mangled names of the two signatures for the statepoint intrinsic collide due to the names of the structs and the parameter types involved. The proposed scheme avoids this collision, as the names become:

declare i32 @llvm.experimental.gc.statepoint.p0f_p0s_si64sf(i64, i32, %si64* ()*, i32, i32, ...)
declare i32 @llvm.experimental.gc.statepoint.p0f_p0s_ssi64f(i64, i32, %s* (i64)*, i32, i32, ...)

[reames]

LGTM. The explanation on the non-literal form makes sense. I'm still not happy about the break in the naming scheme, but I don't have a better option either.

Just for the record, this really should have been two patches. One was a minor enhancement, the other was a subtle change which required careful thought. Please make sure when you land this that you call out the issue here so that it's obvious to readers.

lib/IR/Function.cpp

if (!ValName || !isIntrinsic()) {
     return;
   }
   IntID = lookupIntrinsicID(ValName);
 }
 
 /// Returns a stable mangling for the type specified for use in the name
 /// mangling scheme used by 'any' types in intrinsic signatures.  The mangling
 /// of named types is simply their name.  Manglings for unnamed types consist
-/// of a prefix ('p' for pointers, 'a' for arrays, 'f_' for functions)
-/// combined with the mangling of their component types.  A vararg function
-/// type will have a suffix of 'vararg'.  Since function types can contain
-/// other function types, we close a function type mangling with suffix 'f'
-/// which can't be confused with it's prefix.  This ensures we don't have
-/// collisions between two unrelated function types. Otherwise, you might
-/// parse ffXX as f(fXX) or f(fX)X.  (X is a placeholder for any other type.)
+/// of a prefix ('p' for pointers, 'a' for arrays, 's_' or 'ls_' for structs,
+/// and 'f_' for functions) combined with the mangling of their component types.
+/// A vararg function type will have a suffix of 'vararg'.
+///
+/// Since function/struct types can contain other function/struct types, we
+/// close a function/struct type mangling with suffix 'f'/'s' which can't be
+/// confused with its prefix.  This ensures we don't have collisions between two
+/// unrelated function/struct types. Otherwise, you might parse ffXX as f(fXX)
+/// or f(fX)X.  (X is a placeholder for any other type.)
+///
 /// Manglings of integers, floats, and vectors ('i', 'f', and 'v' prefix in most
 /// cases) fall back to the MVT codepath, where they could be mangled to
 /// 'x86mmx', for example; matching on derived types is not sufficient to mangle
 /// everything.
 static std::string getMangledTypeStr(Type* Ty) {
   std::string Result;
   if (PointerType* PTyp = dyn_cast<PointerType>(Ty)) {
     Result += "p" + llvm::utostr(PTyp->getAddressSpace()) +
       getMangledTypeStr(PTyp->getElementType());
   } else if (ArrayType* ATyp = dyn_cast<ArrayType>(Ty)) {
     Result += "a" + llvm::utostr(ATyp->getNumElements()) +
       getMangledTypeStr(ATyp->getElementType());
   } else if (StructType* STyp = dyn_cast<StructType>(Ty)) {
-    assert(!STyp->isLiteral() && "TODO: implement literal types");
-    Result += STyp->getName();
+    if (STyp->isLiteral()) {
+      Result += "ls_";
+      for (Type* Ty : STyp->elements())
+        Result += getMangledTypeStr(Ty);
+    } else
+      Result += "s_" + STyp->getName().str();
+    // Ensure nested struct types are distinguishable.
+    Result += "s";
   } else if (FunctionType* FT = dyn_cast<FunctionType>(Ty)) {
     Result += "f_" + getMangledTypeStr(FT->getReturnType());
     for (size_t i = 0; i < FT->getNumParams(); i++)
       Result += getMangledTypeStr(FT->getParamType(i));
     if (FT->isVarArg())
       Result += "vararg";
     // Ensure nested function types are distinguishable.
     Result += "f"; 

test/CodeGen/Generic/overloaded-intrinsic-name.ll

define <3 x i32>* @test_vAny(<3 x i32>* %v) gc "statepoint-example" {
        ret <3 x i32>* %v-new
 }
 
 %struct.test = type { i32, i1 }
 
 ; struct
 define %struct.test* @test_struct(%struct.test* %v) gc "statepoint-example" {
        %tok = call i32 (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1f(i64 0, i32 0, i1 ()* @return_i1, i32 0, i32 0, i32 0, i32 0, %struct.test* %v)
-       %v-new = call %struct.test* @llvm.experimental.gc.relocate.p0struct.test(i32 %tok,  i32 7, i32 7)
+       %v-new = call %struct.test* @llvm.experimental.gc.relocate.p0s_struct.tests(i32 %tok,  i32 7, i32 7)
        ret %struct.test* %v-new
 }
 
+; struct literal
+define { i64, float } @test_struct_literal(i64 %v0, float %v1) gc "statepoint-example" {
+       %tok = call i32 (i64, i32, { i64, float } ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_ls_i64f32sf(i64 0, i32 0, { i64, float } ()* @return_ls, i32 0, i32 0, i32 0, i32 0)
+       %v-new = call { i64, float } @llvm.experimental.gc.result.ls_i64f32s(i32 %tok)
+       ret { i64, float } %v-new
+}
+
 declare zeroext i1 @return_i1()
 declare i32 @llvm.experimental.gc.statepoint.p0f_i1f(i64, i32, i1 ()*, i32, i32, ...)
 declare i32* @llvm.experimental.gc.relocate.p0i32(i32, i32, i32)
 declare float* @llvm.experimental.gc.relocate.p0f32(i32, i32, i32)
 declare [3 x i32]* @llvm.experimental.gc.relocate.p0a3i32(i32, i32, i32)
 declare <3 x i32>* @llvm.experimental.gc.relocate.p0v3i32(i32, i32, i32)
-declare %struct.test* @llvm.experimental.gc.relocate.p0struct.test(i32, i32, i32)
+declare %struct.test* @llvm.experimental.gc.relocate.p0s_struct.tests(i32, i32, i32)
+
+declare { i64, float } @return_ls()
+declare i32 @llvm.experimental.gc.statepoint.p0f_ls_i64f32sf(i64, i32, { i64, float } ()*, i32, i32, ...)
+declare { i64, float } @llvm.experimental.gc.result.ls_i64f32s(i32)

test/CodeGen/X86/statepoint-call-lowering.ll

 
 define %struct @test_struct_return() gc "statepoint-example" {
 ; CHECK-LABEL: test_struct_return
 ; CHECK: pushq %rax
 ; CHECK: callq return_struct
 ; CHECK: popq %rcx
 ; CHECK: retq
 entry:
-  %safepoint_token = tail call i32 (i64, i32, %struct ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_structf(i64 0, i32 0, %struct ()* @return_struct, i32 0, i32 0, i32 0, i32 0)
-  %call1 = call %struct @llvm.experimental.gc.result.struct(i32 %safepoint_token)
+  %safepoint_token = tail call i32 (i64, i32, %struct ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_s_structsf(i64 0, i32 0, %struct ()* @return_struct, i32 0, i32 0, i32 0, i32 0)
+  %call1 = call %struct @llvm.experimental.gc.result.s_structs(i32 %safepoint_token)
   ret %struct %call1
 }
 
 define i1 @test_relocate(i32 addrspace(1)* %a) gc "statepoint-example" {
 ; CHECK-LABEL: test_relocate
 ; Check that an ununsed relocate has no code-generation impact
 ; CHECK: pushq %rax
 ; CHECK: callq return_i1

 declare i32 @llvm.experimental.gc.result.i32(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_p0i32f(i64, i32, i32* ()*, i32, i32, ...)
 declare i32* @llvm.experimental.gc.result.p0i32(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_f32f(i64, i32, float ()*, i32, i32, ...)
 declare float @llvm.experimental.gc.result.f32(i32)
 
-declare i32 @llvm.experimental.gc.statepoint.p0f_structf(i64, i32, %struct ()*, i32, i32, ...)
-declare %struct @llvm.experimental.gc.result.struct(i32)
+declare i32 @llvm.experimental.gc.statepoint.p0f_s_structsf(i64, i32, %struct ()*, i32, i32, ...)
+declare %struct @llvm.experimental.gc.result.s_structs(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_isVoidi32varargf(i64, i32, void (i32, ...)*, i32, i32, ...)
 
 declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32, i32, i32)