Implement intrinsic mangling for literal struct types. Fixes PR 31921

[• dberlin]

Summary

Predicateinfo requires an ugly workaround to try to avoid literal
struct types due to the intrinsic mangling not being implemented.
This workaround actually does not work in all cases (you can hit the
assert by bootstrapping with -print-predicateinfo), and can't be made
to work without DFS'ing the type (IE copying getMangledStr and using a
version that detects if it would crash).

Rather than do that, i just implemented the mangling. It seems
simple, since they are unified structurally.

Looking at the overloaded-mangling testcase we have, it actually turns
out the gc intrinsics will *also* crash if you try to use a literal
struct. Thus, the testcase added fails before this patch, and works
after, without needing to resort to predicateinfo.

[chandlerc]

lib/IR/Function.cpp

538 ↗

(On Diff #88303)

Uh.... Am I the only one horrified that we didn't prefix this with anything?

Won't this cause types named 's...' to potentially collide with literal types? Yeah, pretty sure it will. It's not unique to literal types.

Let's take a look at:

%i32 = type { i32 }

define %i32* @test_struct(%i32* %v) gc "statepoint-example" {
entry:
  %tok = call token (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, %i32* %v)
  %v-new = call %i32* @llvm.experimental.gc.relocate.p0i32(token %tok,  i32 7, i32 7)
  ret %i32* %v-new
}

declare zeroext i1 @return_i1()
declare token @llvm.experimental.gc.statepoint.p0f_i1f(i64, i32, i1 ()*, i32, i32, ...)
declare %i32* @llvm.experimental.gc.relocate.p0i32(token, i32, i32)
;declare i32* @llvm.experimental.gc.relocate.p0i32(token, i32, i32)

Also, I think we need to ensure nested types are distinguishable here much like below with function types.

[• dberlin]

lib/IR/Function.cpp

538 ↗

(On Diff #88303)

I just kinda assumed people knew what they were doing in the existing code :)
In any case, updated to prefix structs with s_ in the struct case, and sl_ in the literal case.
We end structs with "s" to be distinguishable.

[• dberlin]

• Update for review comments

Updating D29925: Implement intrinsic mangling for literal struct types.

Fixes PR 31921

[majnemer]

lib/IR/Function.cpp

542–543 ↗

(On Diff #88308)

Range based loop over STyp->elements()?

[• dberlin]

• Move to range based for-loop

Updating D29925: Implement intrinsic mangling for literal struct types.

Fixes PR 31921

[• dberlin]

Since this is going to break every file that uses the old struct mangling, i am splitting it to leave the structs alone for now, because i don't have time to try to figure out an auto-upgrade strategy.

[• dberlin]

• Auto-upgrade by regenerating names when necesseary. Note: we can't really detect if the only change is the name, but it seems like this is the best we can do.

Updating D29925: Implement intrinsic mangling for literal struct types.

Fixes PR 31921

[• dberlin]

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

73 ↗

(On Diff #88452)

Note that these will assert if they don't end up with the right name, so CHECK lines are not strictly necessary.

[chandlerc]

This at least seems like a step in the right direction, so LGTM. Maybe touch base with @majnemer before landing though in case he sees something I don't.

lib/IR/AutoUpgrade.cpp

492–493 ↗

(On Diff #88452)

Scan the types to see if any are pointers to structs before doing this?

[• dberlin]

lib/IR/AutoUpgrade.cpp

492–493 ↗

(On Diff #88452)

Unfortunately, that won't work.
In fact, that was my original workaround in predicateinfo that broke.
There are two problems:

1. ssa_copy is anytype, so it can take a literal struct directly, which will still hit the assert.

But hey, we could special case that one.

There's a much worse problem though:

2. The mangling is recursive over array types, struct type, and function arguments/return types, so it's not enough to identify pointers to structs.

They can be embedded *anywhere*.

So, for example, a pointer to a function type that takes a literal struct as an argument would still hit the old assert (and still be changed in mangling)
Or a pointer to function that has a return type of an array of pointers to functions that take literal structs ...
;(

Essentially, the only way to avoid this check is either:

A. We try to name match all the broken intrinsics and add any we missed over time.
B. We give up and just use it like it is.

I am happy to do A if we are concerned about the compile time cost

[chandlerc]

lib/IR/AutoUpgrade.cpp

492–493 ↗

(On Diff #88452)

Wow. Just wow.

I wouldn't bother with name matching. The intrinsics i'm most concerned with will match (mem*).

Maybe we could check for iN and iN* parameters and skip it. Maybe not worth it.

Actually, yeah, why not just try auto upgrading a large bitcode file and see how much (if any) time is spent here? If none, we can revisit this if ever someone finds bitcode that has enough intrinsics to matter. I'm crossing my fingers and hoping that this just doesn't matter. =]

[• dberlin]

Okay,so i tried this on large bc files (all of clang, etc) i have around.

The amount of time spent in remangling is almost not measureable.
For clang, it claims 100 microseconds.
I'm not sure it has that kind of measurement accuracy, but i can't find a large testcase where i can measure any real amount of time spent here.

So i'm going to commit this, but we should definitely revert and try to focus it more if we do find a testcase that matters.

(the worrying code i can think of would be millions of lines of avx512 or neon intrinsics or something)

[chandlerc]

In D29925#677860, @dberlin wrote:

Okay,so i tried this on large bc files (all of clang, etc) i have around.

The amount of time spent in remangling is almost not measureable.
For clang, it claims 100 microseconds.
I'm not sure it has that kind of measurement accuracy, but i can't find a large testcase where i can measure any real amount of time spent here.

So i'm going to commit this, but we should definitely revert and try to focus it more if we do find a testcase that matters.

This sounds great to me. I like the numbers you found (IE, it is doing work, but the work doesn't seem to consume enough time).

LGTM!

[materi]

Hi!

I think this commit is causing a bug in my out-of-tree target. From opt -lint I started seeing messages like this:

Undefined behavior: Caller and callee calling convention differ
  %1 = call %rec8 @llvm.phx.llmac.i32.u32.s_rec8s(i32 %_tmp1, i32 %_tmp2, i32 %_tmp5, i32 %_tmp4), !dbg !10 (tmp.c:4:15)

The calling convention is dropped in UpgradeIntrinsicCall. It looks like this is easy to fix by just transferring the calling convention to the created CallInst here, is this correct?

llvm/trunk/lib/IR/AutoUpgrade.cpp

1837

The calling convention of CI does not carry over to the new call here.

[• dberlin]

Sorry about that.
I wasn't aware intrinsics were allowed to have a calling convention, since they are internal to llvm.
No intrinsic defined in Intrinsics* has one, from what i can tell, and as you can see, the code we use to upgrade generically does not do anything with calling convention either.
I'm not even sure what it would mean for an intrinsic to have a calling convention (instead of something it *lowers to* to have a calling convention).

Assuming our position is that intrinsics themselves, can have a calling convention, there are two ways to fix this.

We could just replace the called function instead of calling replaceAllUsesWith. Not sure why we don't, but CI->setCalledFunction(NewFn) should work (if we do that, we should stop renaming it .old).

Otherwise, if we really care, we should probably copy both the attributes and the calling convention.

[materi]

In D29925#687131, @dberlin wrote:

Sorry about that.
I wasn't aware intrinsics were allowed to have a calling convention, since they are internal to llvm.
No intrinsic defined in Intrinsics* has one, from what i can tell, and as you can see, the code we use to upgrade generically does not do anything with calling convention either.
I'm not even sure what it would mean for an intrinsic to have a calling convention (instead of something it *lowers to* to have a calling convention).

Maybe we are just doing something stupid when we add a calling convention to our intrinsics.

I don't know exactly why we have this, maybe they are just an accident. Thanks for pointing it out!

Assuming our position is that intrinsics themselves, can have a calling convention, there are two ways to fix this.

I'll experiment with removing all calling conventions from our intrinsics (in the out-of-tree target). It's possible our position should be that CC is not allowed.

[sanjoy]

In D29925#687131, @dberlin wrote:

I wasn't aware intrinsics were allowed to have a calling convention, since they are internal to llvm.
No intrinsic defined in Intrinsics* has one, from what i can tell, and as you can see, the code we use to upgrade generically does not do anything with calling convention either.
I'm not even sure what it would mean for an intrinsic to have a calling convention (instead of something it *lowers to* to have a calling convention).

It is used for statepoints and patchpoints. call cconv42 @statepoint(@foo) is lowered to a call to @foo, and cconv42 is interpreted as the calling convention for the lowered call to @foo.

[• dberlin]

Patch in D30422

llvm/trunk/lib/IR/AutoUpgrade.cpp

if (IsX86 && Name.startswith("xop.vpermil2")) { // Added in 3.9
           Permil2ID = Intrinsic::x86_xop_vpermil2ps_256;
         NewFn = Intrinsic::getDeclaration(F->getParent(), Permil2ID);
         return true;
       }
     }
     break;
   }
   }
+  // Remangle our intrinsic since we upgrade the mangling
+  auto Result = llvm::Intrinsic::remangleIntrinsicFunction(F);
+  if (Result != None) {
+    NewFn = Result.getValue();
+    return true;
+  }
 
   //  This may not belong here. This function is effectively being overloaded
   //  to both detect an intrinsic which needs upgrading, and to provide the
   //  upgraded form of the intrinsic. We should perhaps have two separate
   //  functions for this.
   return false;
 }
 

     return;
   }
 
   std::string Name = CI->getName();
   if (!Name.empty())
     CI->setName(Name + ".old");
 
   switch (NewFn->getIntrinsicID()) {
-  default:
-    llvm_unreachable("Unknown function for CallInst upgrade.");
+  default: {
+    // Handle generic mangling change, but nothing else
+    assert(
+        (CI->getCalledFunction()->getName() != NewFn->getName()) &&
+        "Unknown function for CallInst upgrade and isn't just a name change");
+    SmallVector<Value *, 4> Args(CI->arg_operands().begin(),
+                                 CI->arg_operands().end());
+    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, Args));
+    CI->eraseFromParent();
+    return;
+  }
 
   case Intrinsic::arm_neon_vld1:
   case Intrinsic::arm_neon_vld2:
   case Intrinsic::arm_neon_vld3:
   case Intrinsic::arm_neon_vld4:
   case Intrinsic::arm_neon_vld2lane:
   case Intrinsic::arm_neon_vld3lane:
   case Intrinsic::arm_neon_vld4lane:

llvm/trunk/lib/IR/Function.cpp

 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");
+  } else if (StructType *STyp = dyn_cast<StructType>(Ty)) {
+    if (!STyp->isLiteral()) {
+      Result += "s_";
       Result += STyp->getName();
-  } else if (FunctionType* FT = dyn_cast<FunctionType>(Ty)) {
+    } else {
+      Result += "sl_";
+      for (auto Elem : STyp->elements())
+        Result += getMangledTypeStr(Elem);
+    }
+    // Ensure nested structs 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"; 
   } else if (isa<VectorType>(Ty))

llvm/trunk/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 token (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(token %tok,  i32 7, i32 7)
+       %v-new = call %struct.test* @llvm.experimental.gc.relocate.p0s_struct.tests(token %tok,  i32 7, i32 7)
        ret %struct.test* %v-new
 }
 
+; literal struct with nested literal struct
+define {i64, i64, {i64} }* @test_literal_struct({i64, i64, {i64}}* %v) gc "statepoint-example" {
+       %tok = call token (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, {i64, i64, {i64}} *%v)
+       %v-new = call {i64, i64, {i64}}* @llvm.experimental.gc.relocate.p0sl_i64i64sl_i64ss.test(token %tok,  i32 7, i32 7)
+       ret {i64, i64, {i64}}* %v-new
+}
+; struct with a horrible name, broken when structs were unprefixed
+%i32 = type { i32 }
+
+define %i32* @test_i32_struct(%i32* %v) gc "statepoint-example" {
+entry:
+      %tok = call token (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, %i32* %v)
+      %v-new = call %i32* @llvm.experimental.gc.relocate.p0s_i32s(token %tok,  i32 7, i32 7)
+      ret %i32* %v-new
+}
+; completely broken intrinsic naming due to needing remangling. Just use random naming to test
+
+define %i32* @test_broken_names(%i32* %v) gc "statepoint-example" {
+entry:
+      %tok = call token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.deadbeef(i64 0, i32 0, i1 ()* @return_i1, i32 0, i32 0, i32 0, i32 0, %i32* %v)
+      %v-new = call %i32* @llvm.experimental.gc.relocate.beefdead(token %tok,  i32 7, i32 7)
+      ret %i32* %v-new
+}
 declare zeroext i1 @return_i1()
 declare token @llvm.experimental.gc.statepoint.p0f_i1f(i64, i32, i1 ()*, i32, i32, ...)
 declare i32* @llvm.experimental.gc.relocate.p0i32(token, i32, i32)
 declare float* @llvm.experimental.gc.relocate.p0f32(token, i32, i32)
 declare [3 x i32]* @llvm.experimental.gc.relocate.p0a3i32(token, i32, i32)
 declare <3 x i32>* @llvm.experimental.gc.relocate.p0v3i32(token, i32, i32)
-declare %struct.test* @llvm.experimental.gc.relocate.p0struct.test(token, i32, i32)
+declare %struct.test* @llvm.experimental.gc.relocate.p0s_struct.tests(token, i32, i32)
+declare {i64, i64, {i64}}* @llvm.experimental.gc.relocate.p0sl_i64i64sl_i64ss.test(token, i32, i32)
+declare %i32* @llvm.experimental.gc.relocate.p0s_i32s(token, i32, i32)
+declare %i32* @llvm.experimental.gc.relocate.beefdead(token, i32, i32)
+declare token @llvm.experimental.gc.statepoint.deadbeef(i64, i32, i1 ()*, i32, i32, ...)

llvm/trunk/test/LTO/X86/remangle_intrinsics_tbaa.ll

 ; RUN: llvm-as %s -o %t1.bc
 ; RUN: llvm-as %p/Inputs/remangle_intrinsics_tbaa.ll -o %t2.bc
 ; RUN: llvm-link -disable-lazy-loading %t2.bc %t1.bc -S | FileCheck %s
 
 ; Verify that we correctly rename the intrinsic and don't crash
-; CHECK: @llvm.masked.store.v4p0some_named_struct.0.p0v4p0some_named_struct.0
+; CHECK: @llvm.masked.store.v4p0s_some_named_struct.0s.p0v4p0s_some_named_struct.0s
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 %some_named_struct = type { i8 }
 
 define void @foo(%some_named_struct*) {
   call void @llvm.masked.store.v4p0some_named_struct.p0v4p0some_named_struct(<4 x %some_named_struct*> undef, <4 x %some_named_struct*>* undef, i32 8, <4 x i1> undef), !tbaa !5

llvm/trunk/unittests/Linker/LinkModulesTest.cpp

TEST_F(LinkModuleTest, RemangleIntrinsics) {
 
   // We load two modules inside the same context C. In both modules there is a
   // "struct.rtx_def" type. In the module loaded the second (Bar) this type will
   // be renamed to "struct.rtx_def.0". Check that the intrinsics which have this
   // type in the signature are properly remangled.
   const char *FooStr =
     "%struct.rtx_def = type { i16 }\n"
     "define void @foo(%struct.rtx_def* %a, i8 %b, i32 %c) {\n"
-    "  call void  @llvm.memset.p0struct.rtx_def.i32(%struct.rtx_def* %a, i8 %b, i32 %c, i32 4, i1 true)\n"
+    "  call void  @llvm.memset.p0s_struct.rtx_defs.i32(%struct.rtx_def* %a, i8 %b, i32 %c, i32 4, i1 true)\n"
     "  ret void\n"
     "}\n"
-    "declare void @llvm.memset.p0struct.rtx_def.i32(%struct.rtx_def*, i8, i32, i32, i1)\n";
+    "declare void @llvm.memset.p0s_struct.rtx_defs.i32(%struct.rtx_def*, i8, i32, i32, i1)\n";
 
   const char *BarStr =
     "%struct.rtx_def = type { i16 }\n"
     "define void @bar(%struct.rtx_def* %a, i8 %b, i32 %c) {\n"
-    "  call void  @llvm.memset.p0struct.rtx_def.i32(%struct.rtx_def* %a, i8 %b, i32 %c, i32 4, i1 true)\n"
+    "  call void  @llvm.memset.p0s_struct.rtx_defs.i32(%struct.rtx_def* %a, i8 %b, i32 %c, i32 4, i1 true)\n"
     "  ret void\n"
     "}\n"
-    "declare void @llvm.memset.p0struct.rtx_def.i32(%struct.rtx_def*, i8, i32, i32, i1)\n";
+    "declare void @llvm.memset.p0s_struct.rtx_defs.i32(%struct.rtx_def*, i8, i32, i32, i1)\n";
 
   std::unique_ptr<Module> Foo = parseAssemblyString(FooStr, Err, C);
   assert(Foo);
   ASSERT_TRUE(Foo.get());
   // Foo is loaded first, so the type and the intrinsic have theis original
   // names.
-  ASSERT_TRUE(Foo->getFunction("llvm.memset.p0struct.rtx_def.i32"));
-  ASSERT_FALSE(Foo->getFunction("llvm.memset.p0struct.rtx_def.0.i32"));
+  ASSERT_TRUE(Foo->getFunction("llvm.memset.p0s_struct.rtx_defs.i32"));
+  ASSERT_FALSE(Foo->getFunction("llvm.memset.p0s_struct.rtx_defs.0.i32"));
 
   std::unique_ptr<Module> Bar = parseAssemblyString(BarStr, Err, C);
   assert(Bar);
   ASSERT_TRUE(Bar.get());
   // Bar is loaded after Foo, so the type is renamed to struct.rtx_def.0. Check
   // that the intrinsic is also renamed.
-  ASSERT_FALSE(Bar->getFunction("llvm.memset.p0struct.rtx_def.i32"));
-  ASSERT_TRUE(Bar->getFunction("llvm.memset.p0struct.rtx_def.0.i32"));
+  ASSERT_FALSE(Bar->getFunction("llvm.memset.p0s_struct.rtx_defs.i32"));
+  ASSERT_TRUE(Bar->getFunction("llvm.memset.p0s_struct.rtx_def.0s.i32"));
 
   // Link two modules together.
   auto Dst = llvm::make_unique<Module>("Linked", C);
   ASSERT_TRUE(Dst.get());
   Ctx.setDiagnosticHandler(expectNoDiags);
   bool Failed = Linker::linkModules(*Foo, std::move(Bar));
   ASSERT_FALSE(Failed);
 
   // "struct.rtx_def" from Foo and "struct.rtx_def.0" from Bar are isomorphic
   // types, so they must be uniquified by linker. Check that they use the same
   // intrinsic definition.
-  Function *F = Foo->getFunction("llvm.memset.p0struct.rtx_def.i32");
+  Function *F = Foo->getFunction("llvm.memset.p0s_struct.rtx_defs.i32");
   ASSERT_EQ(F->getNumUses(), (unsigned)2);
 }
 
 } // end anonymous namespace