[IR] Require intrinsic struct return type to be anonymous

[nikic]

Summary

This is an alternative to D122376. Rather than working around the problem, this patch requires that struct return type in intrinsics are anonymous/literal and adds auto-upgrade code to convert existing uses of intrinsics with named struct types.

This also fixes https://github.com/llvm/llvm-project/issues/37891.

[nikic]

llvm/lib/IR/AutoUpgrade.cpp

591

This is a remangling upgrade, but previously was done manually to preserve the named struct type. Now we should use the generic code.

llvm/lib/IR/Function.cpp

1487

This is a bit of an ugly edge case, because "externref" type used by some wasm intrinsic is defined as pointer to empty struct, where "empty struct" is a named type in practice. I didn't bother handling this case, because this edge-case will go away with opaque pointers anyway, and as such doesn't seem worth addressing with additional machinery right now.

llvm/test/CodeGen/X86/2009-04-12-FastIselOverflowCrash.ll

8

The auto-upgrade does work fine for these tests, but they are testing FastISel and the resulting extract+insert+extract sequence is not supported by FastISel.

[aeubanks]

intrinsics-struct-upgrade.ll is failing on the buildbots

[dexonsmith]

I think dropping struct names from intrinsic return types is a bit unfortunate. I wonder if there's a way to keep them?

For context, I've been imagining the following transition for named structs:

1. Opaque pointers. (This removes all cycles in the Type graph.)
2. Refactor named structs into a pointer to "unnamed struct" (immutable) + "a name".
3. "Remove" named structs, moving the name field to a map in Module (and, maybe, a map per-Function to override).
   • getType() always refers to an "unnamed struct".
   • Textual IR would print the name assigned by the owner (instruction checks map in Function (if we have that), then Module). Instruction::print() can do the same.
   • (Requires (1), which removes all cycles in the Type graph.)
4. (All Types are now immutable. Refactor accordingly.)
5. (Make get() and create() thread-safe for types by using a thread-safe insert-only data structure.)
6. (various other things...)

(More context than needed/useful, probably; also, I'm aware that (3) has caveats / hasn't been proposed / etc.)

My point: once (2) is done, it would be trivial to check for two different named types whether they were structurally equivalent. At point, you could have module-specific names on structs returned from intrinsics.

I wonder if doing (2) now (before opaque pointers is done) would be possible/useful, and if it'd help keeping names here?

Or, maybe, names on struct types returned by intrinsics just aren't very valuable? (since they're rare / small / easy enough to reason about?)

[nikic]

@dexonsmith In the end, the part we care about is that that there is a unique mapping between intrinsic names and intrinsic function types. Even if we represented named structs as a type containing an anonymous struct and a name, that would still mean we could have two different function types for one intrinsic name. The only way to avoid that would be to actually include the struct type in intrinsic mangling, so we'd have a different intrinsic for (say) llvm.sadd.with.overflow.i32.s_anon returning an anonymous struct and llvm.sadd.with.overflow.i32.s_0 returning a %0 named struct. That's possible, but I don't think its worthwhile.

Conversely, if we move towards only having anonymous structs and a name assigned in a separate map, then that name is no longer part of the Type, and we only have the one intrinsic signature. The name is just a question of pretty rendering. Though TBH I am not sure how much sense that approach makes -- I think named types are primarily of interest if you can use different names for the "same" type, while this would limit you to one.

Generally, I'd say that struct types in LLVM are on the way out. Opaque pointers make the first major step towards that, and if we could get rid of all the other unnecessary type uses in LLVM (GEP, alloca, byval, etc) the only remaining use for struct types would be functions with multiple return values. But well, we're still rather far away from this bright future :)

Or, maybe, names on struct types returned by intrinsics just aren't very valuable? (since they're rare / small / easy enough to reason about?)

Yeah, I think when it comes to IR intrinsics, struct returns are generally only used to return two values. I don't think people use named return types for this in practice anymore, because it doesn't really work for non-trivial situations (i.e. whenever LLVM creates new intrinsic calls, as it does with with.overflow intrinsics).

[dexonsmith]

Thanks for thinking it over; SGTM. (I haven't reviewed details of this patch, but don't let my comment block landing it.)

In D122471#3409501, @nikic wrote:

I think named types are primarily of interest if you can use different names for the "same" type, while this would limit you to one.

IMO, named types also add value when describing nested structures. E.g., this:

declare {i32, {i32, {i32, i32}, {i32, i32}, i32}, i32, {i32, i32}} @thing2er()

is harder to read than this:

%i32pair = {i32, i32}
%thing1 = {i32, %i32pair, %i32pair, i32}
%thing2 = {i32, %thing1, i32, %i32pair}

declare %thing2 @thing2er()

IMO, after opaque pointers land, type names will just be helpful comments for debugging IR, like value names; not really a core part of the type system. But I take your point that it's useful to have multiple names for the same type. I'll keep thinking; maybe there's a good way to keep that.

[nikic]

Hrm, unfortunately this fails clang/test/CodeGenCUDA/texture.cu, which does:

__attribute__((device)) v4f tex2d_ld(texture<float, 2, ElementType>, float, float) asm("llvm.nvvm.tex.unified.2d.v4f32.f32");
__attribute__((device)) v4f tex2d_ld(texture<float, 2, NormalizedFloat>, int, int) asm("llvm.nvvm.tex.unified.2d.v4f32.s32");

TIL about the asm attribute, though thankfully clang docs are very explicit about this usage being unsupported:

While it is possible to use this attribute to name a special symbol used internally by the compiler, such as an LLVM intrinsic, this is neither recommended nor supported and may cause the compiler to crash or miscompile. Users who wish to gain access to intrinsic behavior are strongly encouraged to request new builtin functions.

I'm not entirely sure how to resolve this -- possibly the right action here is to simply delete the test, as it's an unsupported usage. An alternative would be to do intrinsic auto-upgrade during frontend codegen (this is something rustc does for example), but given the fact that this should only be relevant for this asm construct, and that such usage is explicitly unsupported, we probably shouldn't go out of our way to make it work.

[nikic]

Mark clang/test/CodeGenCUDA/texture.cu as XFAIL.

cc @hliao @tra in case you want to export builtins for these intrinsics before I land this.

[tra]

I agree that that texture.cu test is broken. Using inline IR asm is a rather questionable thing to do. I'll need to figure out a better way to test it, but it should not stop this patch. I think in practice we're not relying on those intrinsics and use inline asm to do texture fetches, so the change should not affect end users.

[uabelho]

Hello,

No idea what is happening yet but I've seen some pretty nasty memory consumption by opt that I bisected to this patch.

Any idea what could be going on?

I'll see if I can see if I can extract some reproducer.

[uabelho]

In D122471#3416528, @uabelho wrote:

Hello,

No idea what is happening yet but I've seen some pretty nasty memory consumption by opt that I bisected to this patch.

Any idea what could be going on?

I'll see if I can see if I can extract some reproducer.

It seems like it's the actual reading of the input file that hangs and just eats more and more memory. So

opt foo.bc -o foo.opt.bc

hangs, and this happens when the input contains a call to an intrinsic we've added downstream that returns a named struct type.

If I extract ll output from clang it looks like

%struct.__divm32_t_tag = type { i32, i32 }
[...]
  %0 = call %struct.__divm32_t_tag @llvm.phx.divm.u32.s_struct.__divm32_t_tags(i32 2, i32 9), !dbg !10

and opt happily reads that, but if I output bc from clang and then feed opt it hangs.

Do we need to update our intrinsic definitions in some way?

[nikic]

@uabelho You shouldn't need to update intrinsic definitions (though you might want to update how you codegen the intrinsics). It sounds like the auto-upgrade code is going into an infinite loop in your case, but it's not really obvious to me how/why that would happen (or why it would happen only for bitcode but not IR).

[nikic]

Oh wait, do I see correctly that your intrinsic returns an overloaded type that happens to be a struct, rather than returning a struct defined in the intrinsic definition? I hadn't considered that case, and it's possible the auto-upgrade code is broken for that.

[nikic]

@uabelho Can you please check whether https://github.com/llvm/llvm-project/commit/d6887256c2cae1b1b721bd47459be6d86003db6f fixes the problem you're seeing?

[uabelho]

In D122471#3416797, @nikic wrote:

@uabelho Can you please check whether https://github.com/llvm/llvm-project/commit/d6887256c2cae1b1b721bd47459be6d86003db6f fixes the problem you're seeing?

Hi @nikic

Yes, that patch solves the problem. Great, thanks!

[goncharov]

This comment was removed by goncharov.

clang/test/CodeGenCUDA/texture.cu

 // REQUIRES: x86-registered-target
 // REQUIRES: nvptx-registered-target
 
 // RUN: %clang_cc1 -std=c++11 -fcuda-is-device -triple nvptx64-nvidia-cuda -emit-llvm -o - %s | FileCheck --check-prefix=DEVICE %s
 // RUN: echo "GPU binary would be here" > %t
 // RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux-gnu -target-sdk-version=8.0 -fcuda-include-gpubinary %t -emit-llvm -o - %s | FileCheck --check-prefix=HOST %s
 
+// Accessing nvvm intrinsics in this way no longer works.
+// XFAIL: *
+
 struct textureReference {
   int desc;
 };
 
 enum ReadMode {
   ElementType = 0,
   NormalizedFloat = 1
 };

llvm/lib/IR/AutoUpgrade.cpp

if (Name.startswith("arm.neon.vclz")) {
                                "llvm.ctlz." + Name.substr(14), F->getParent());
       return true;
     }
     if (Name.startswith("arm.neon.vcnt")) {
       NewFn = Intrinsic::getDeclaration(F->getParent(), Intrinsic::ctpop,
                                         F->arg_begin()->getType());
       return true;
     }
-    static const Regex vldRegex("^arm\\.neon\\.vld([1234]|[234]lane)\\.v[a-z0-9]*$");
-    if (vldRegex.match(Name)) {
-      auto fArgs = F->getFunctionType()->params();
-      SmallVector<Type *, 4> Tys(fArgs.begin(), fArgs.end());
-      // Can't use Intrinsic::getDeclaration here as the return types might
-      // then only be structurally equal.
-      FunctionType* fType = FunctionType::get(F->getReturnType(), Tys, false);
-      StringRef Suffix =
-          F->getContext().supportsTypedPointers() ? "p0i8" : "p0";
-      NewFn = Function::Create(fType, F->getLinkage(), F->getAddressSpace(),
-                               "llvm." + Name + "." + Suffix, F->getParent());
-      return true;
-    }
     static const Regex vstRegex("^arm\\.neon\\.vst([1234]|[234]lane)\\.v[a-z0-9]*$");
     if (vstRegex.match(Name)) {
       static const Intrinsic::ID StoreInts[] = {Intrinsic::arm_neon_vst1,
                                                 Intrinsic::arm_neon_vst2,
                                                 Intrinsic::arm_neon_vst3,
                                                 Intrinsic::arm_neon_vst4};
 
       static const Intrinsic::ID StoreLaneInts[] = {
case 'v': {
     }
     break;
   }
 
   case 'x':
     if (UpgradeX86IntrinsicFunction(F, Name, NewFn))
       return true;
   }
+
+  if (auto *ST = dyn_cast<StructType>(F->getReturnType())) {
+    if (!ST->isLiteral() || ST->isPacked()) {
+      // Replace return type with literal non-packed struct.
+      auto *FT = F->getFunctionType();
+      auto *NewST = StructType::get(ST->getContext(), ST->elements());
+      auto *NewFT = FunctionType::get(NewST, FT->params(), FT->isVarArg());
+      std::string Name = F->getName().str();
+      rename(F);
+      NewFn = Function::Create(NewFT, F->getLinkage(), F->getAddressSpace(),
+                               Name, F->getParent());
+
+      // The new function may also need remangling.
+      if (auto Result = llvm::Intrinsic::remangleIntrinsicFunction(F))
+        NewFn = *Result;
+      return true;
+    }
+  }
+
   // 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

     }
 
     if (Rep)
       CI->replaceAllUsesWith(Rep);
     CI->eraseFromParent();
     return;
   }
 
-  const auto &DefaultCase = [&NewFn, &CI]() -> void {
-    // Handle generic mangling change, but nothing else
+  const auto &DefaultCase = [&]() -> void {
+    if (CI->getFunctionType() == NewFn->getFunctionType()) {
+      // Handle generic mangling change.
       assert(
           (CI->getCalledFunction()->getName() != NewFn->getName()) &&
           "Unknown function for CallBase upgrade and isn't just a name change");
       CI->setCalledFunction(NewFn);
+      return;
+    }
+
+    // This must be an upgrade from a named to a literal struct.
+    auto *OldST = cast<StructType>(CI->getType());
+    auto *NewST = cast<StructType>(NewFn->getReturnType());
+    assert(OldST != NewST && "Return type must have changed");
+    assert(OldST->getNumElements() == NewST->getNumElements() &&
+           "Must have same number of elements");
+
+    SmallVector<Value *> Args(CI->args());
+    Value *NewCI = Builder.CreateCall(NewFn, Args);
+    Value *Res = PoisonValue::get(OldST);
+    for (unsigned Idx = 0; Idx < OldST->getNumElements(); ++Idx) {
+      Value *Elem = Builder.CreateExtractValue(NewCI, Idx);
+      Res = Builder.CreateInsertValue(Res, Elem, Idx);
+    }
+    CI->replaceAllUsesWith(Res);
+    CI->eraseFromParent();
+    return;
   };
   CallInst *NewCall = nullptr;
   switch (NewFn->getIntrinsicID()) {
   default: {
     DefaultCase();
     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:
   case Intrinsic::arm_neon_vst1:
   case Intrinsic::arm_neon_vst2:
   case Intrinsic::arm_neon_vst3:
   case Intrinsic::arm_neon_vst4:
   case Intrinsic::arm_neon_vst2lane:
   case Intrinsic::arm_neon_vst3lane:
   case Intrinsic::arm_neon_vst4lane: {
     SmallVector<Value *, 4> Args(CI->args());

llvm/lib/IR/Function.cpp

case IITDescriptor::Vector: {
       return !VT || VT->getElementCount() != D.Vector_Width ||
              matchIntrinsicType(VT->getElementType(), Infos, ArgTys,
                                 DeferredChecks, IsDeferredCheck);
     }
     case IITDescriptor::Pointer: {
       PointerType *PT = dyn_cast<PointerType>(Ty);
       if (!PT || PT->getAddressSpace() != D.Pointer_AddressSpace)
         return true;
-      if (!PT->isOpaque())
+      if (!PT->isOpaque()) {
+        /* Manually consume a pointer to empty struct descriptor, which is
+         * used for externref. We don't want to enforce that the struct is
+         * anonymous in this case. (This renders externref intrinsics
+         * non-unique, but this will go away with opaque pointers anyway.) */
+        if (Infos.front().Kind == IITDescriptor::Struct &&
+            Infos.front().Struct_NumElements == 0) {
+          Infos = Infos.slice(1);
+          return false;
+        }
         return matchIntrinsicType(PT->getNonOpaquePointerElementType(), Infos,
                                   ArgTys, DeferredChecks, IsDeferredCheck);
+      }
       // Consume IIT descriptors relating to the pointer element type.
       // FIXME: Intrinsic type matching of nested single value types or even
       // aggregates doesn't work properly with opaque pointers but hopefully
       // doesn't happen in practice.
       while (Infos.front().Kind == IITDescriptor::Pointer ||
              Infos.front().Kind == IITDescriptor::Vector)
         Infos = Infos.slice(1);
       Infos = Infos.slice(1);
       return false;
     }
 
     case IITDescriptor::Struct: {
       StructType *ST = dyn_cast<StructType>(Ty);
-      if (!ST || ST->getNumElements() != D.Struct_NumElements)
+      if (!ST || !ST->isLiteral() || ST->isPacked() ||
+          ST->getNumElements() != D.Struct_NumElements)
         return true;
 
       for (unsigned i = 0, e = D.Struct_NumElements; i != e; ++i)
         if (matchIntrinsicType(ST->getElementType(i), Infos, ArgTys,
                                DeferredChecks, IsDeferredCheck))
           return true;
       return false;
     }

llvm/test/Bitcode/intrinsics-struct-upgrade.ll

+; RUN: llvm-dis < %s.bc | FileCheck %s
+
+%struct.__neon_int8x8x2_t = type { <8 x i8>, <8 x i8> }
+
+declare %struct.__neon_int8x8x2_t @llvm.aarch64.neon.ld2.v8i8.p0i8(i8*)
+
+; CHECK-LABEL: define %struct.__neon_int8x8x2_t @test_named_struct_return(i8* %A) {
+; CHECK:  %1 = call { <8 x i8>, <8 x i8> } @llvm.aarch64.neon.ld2.v8i8.p0i8(i8* %A)
+; CHECK:  %2 = extractvalue { <8 x i8>, <8 x i8> } %1, 0
+; CHECK:  %3 = insertvalue %struct.__neon_int8x8x2_t poison, <8 x i8> %2, 0
+; CHECK:  %4 = extractvalue { <8 x i8>, <8 x i8> } %1, 1
+; CHECK:  %5 = insertvalue %struct.__neon_int8x8x2_t %3, <8 x i8> %4, 1
+; CHECK:  ret %struct.__neon_int8x8x2_t %5
+
+define %struct.__neon_int8x8x2_t @test_named_struct_return(i8* %A) {
+  %val = call %struct.__neon_int8x8x2_t @llvm.aarch64.neon.ld2.v8i8.p0i8(i8* %A)
+  ret %struct.__neon_int8x8x2_t %val
+}

llvm/test/CodeGen/X86/2009-04-12-FastIselOverflowCrash.ll

 ; radr://6772169
 ; RUN: llc < %s -fast-isel
 ; PR30981
 ; RUN: llc < %s -O0 -mcpu=x86-64 -mattr=+avx512f | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128"
 target triple = "x86_64-apple-darwin10"
-  %0 = type { i32, i1 }   ; type %0
 
-declare %0 @llvm.sadd.with.overflow.i32(i32, i32) nounwind
+declare { i32, i1 } @llvm.sadd.with.overflow.i32(i32, i32) nounwind
 
 define fastcc i32 @test() nounwind {
 entry:
 ; CHECK-LABEL: test:
 ; CHECK:       ## %bb.0:
 ; CHECK-NEXT:    movl $1, [[REG:%e[a-z]+]]
 ; CHECK-NEXT:    addl $0, [[REG]]
 ; CHECK-NEXT:    seto {{%[a-z]+l}}
 ; CHECK:         jo LBB0_2
-  %tmp1 = call %0 @llvm.sadd.with.overflow.i32(i32 1, i32 0)
-  %tmp2 = extractvalue %0 %tmp1, 1
+  %tmp1 = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 1, i32 0)
+  %tmp2 = extractvalue { i32, i1 } %tmp1, 1
   br i1 %tmp2, label %.backedge, label %BB3
 
 BB3:
-  %tmp4 = extractvalue %0 %tmp1, 0
+  %tmp4 = extractvalue { i32, i1 } %tmp1, 0
   br label %.backedge
 
 .backedge:
   ret i32 0
 }

llvm/test/CodeGen/X86/fast-isel-extract.ll

 ; RUN: llc < %s -mtriple x86_64-apple-darwin11 -O0 -fast-isel-abort=1 | FileCheck %s
 
 %struct.x = type { i64, i64 }
-%addovf = type { i32, i1 }
 declare %struct.x @f()
 
 define void @test1(i64*) nounwind ssp {
   %2 = tail call %struct.x @f() nounwind
   %3 = extractvalue %struct.x %2, 0
   %4 = add i64 %3, 10
   store i64 %4, i64* %0
   ret void
define void @test2(i64*) nounwind ssp {
   store i64 %4, i64* %0
   ret void
 ; CHECK-LABEL: test2:
 ; CHECK: callq _f
 ; CHECK-NOT: %rdx
 ; CHECK: addq $10, %rdx
 }
 
-declare %addovf @llvm.sadd.with.overflow.i32(i32, i32) nounwind readnone
+declare { i32, i1 } @llvm.sadd.with.overflow.i32(i32, i32) nounwind readnone
 
 define void @test3(i32 %x, i32 %y, i32* %z) {
-  %r = call %addovf @llvm.sadd.with.overflow.i32(i32 %x, i32 %y)
-  %sum = extractvalue %addovf %r, 0
+  %r = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 %x, i32 %y)
+  %sum = extractvalue { i32, i1 } %r, 0
   %sum3 = mul i32 %sum, 3
-  %bit = extractvalue %addovf %r, 1
+  %bit = extractvalue { i32, i1 } %r, 1
   br i1 %bit, label %then, label %end
   
 then:
   store i32 %sum3, i32* %z
   br label %end
 
 end:
   ret void
 ; CHECK: test3
 ; CHECK: addl
 ; CHECK: seto %al
 ; CHECK: testb $1, %al
 }