[AutoUpgrade] Take unnamed types into account.

[jeroen.dobbelaere]

Summary

Ensure that we provide access to the Module when checking if a rename of an intrinsic is necessary.

This fixes the issue that was detected by https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=32288
(as mentioned by @fhahn), after committing D91250.

[fhahn]

Is there a way to ensure all call sites that may need to handle opaque pointers pass the module? can we add an assertion ensuring the module is passed if the intrinsic takes overloaded pointer types?

[jeroen.dobbelaere]

In D99066#2641043, @fhahn wrote:

Is there a way to ensure all call sites that may need to handle opaque pointers pass the module? can we add an assertion ensuring the module is passed if the intrinsic takes overloaded pointer types?

The easiest way is to always pass the Module when a list of types is provided. But that is not a guarantee that the Module pointer will ever be needed for that specific intrinsic.

We could add an extra assertion that triggers when a pointer type is provided, but no Module is given. But also here, there will be false positives. (Intrinsics that can get a pointer, but not a pointer involving an unnamed type.)
I tried this in out, and got extra failures because of prefetch, masked_load, masked_store, masked_gather, masked_scatter. Can these intrinsics use pointers that involve unnamed types ?

Note: There might also be an issue with LLVMIntrinsicCopyOverloadedName, which does not have a Module parameter. As long as no unnamed types are used, that interface is fine, but it does not support unnamed types.

[fhahn]

In D99066#2641109, @jeroen.dobbelaere wrote:

In D99066#2641043, @fhahn wrote:

Is there a way to ensure all call sites that may need to handle opaque pointers pass the module? can we add an assertion ensuring the module is passed if the intrinsic takes overloaded pointer types?

The easiest way is to always pass the Module when a list of types is provided. But that is not a guarantee that the Module pointer will ever be needed for that specific intrinsic.

How many places would need to be updated? It sounds like there's no nice solution (automatically catch cases that really need the module), but it also seems brittle to have Intrinsic::getName crash in various places because of missing Module arguments.

We could add an extra assertion that triggers when a pointer type is provided, but no Module is given. But also here, there will be false positives. (Intrinsics that can get a pointer, but not a pointer involving an unnamed type.)
I tried this in out, and got extra failures because of prefetch, masked_load, masked_store, masked_gather, masked_scatter. Can these intrinsics use pointers that involve unnamed types ?

llvm.prefetch can be used with opaque types (https://godbolt.org/z/xnjx5b99Y), the other cannot be used with opaque pointers I think, because they also take vector arguments and opaque types cannot be used as vector element types.

Note: There might also be an issue with LLVMIntrinsicCopyOverloadedName, which does not have a Module parameter. As long as no unnamed types are used, that interface is fine, but it does not support unnamed types.

Ok it sounds like we also need a fix to the C api?

[jeroen.dobbelaere]

In D99066#2641561, @fhahn wrote:

How many places would need to be updated? It sounds like there's no nice solution (automatically catch cases that really need the module), but it also seems brittle to have Intrinsic::getName crash in various places because of missing Module arguments.

It's not too much. We better then always require the Module parameter whenever types are provided.

[..]

Note: There might also be an issue with LLVMIntrinsicCopyOverloadedName, which does not have a Module parameter. As long as no unnamed types are used, that interface is fine, but it does not support unnamed types.

Ok it sounds like we also need a fix to the C api?

If we want to support the unnamed types, yes. Can we just require/add an extra Module argument ? Are there any rules for backwards compatibility for the C api ?

[jeroen.dobbelaere]

Dropping in favor of D99173.

llvm/lib/IR/AutoUpgrade.cpp

static bool UpgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
   case 'i':
   case 'l': {
     bool IsLifetimeStart = Name.startswith("lifetime.start");
     if (IsLifetimeStart || Name.startswith("invariant.start")) {
       Intrinsic::ID ID = IsLifetimeStart ?
         Intrinsic::lifetime_start : Intrinsic::invariant_start;
       auto Args = F->getFunctionType()->params();
       Type* ObjectPtr[1] = {Args[1]};
-      if (F->getName() != Intrinsic::getName(ID, ObjectPtr)) {
+      if (F->getName() !=
+          Intrinsic::getName(ID, ObjectPtr, F->getParent(), nullptr)) {
         rename(F);
         NewFn = Intrinsic::getDeclaration(F->getParent(), ID, ObjectPtr);
         return true;
       }
     }
 
     bool IsLifetimeEnd = Name.startswith("lifetime.end");
     if (IsLifetimeEnd || Name.startswith("invariant.end")) {
       Intrinsic::ID ID = IsLifetimeEnd ?
         Intrinsic::lifetime_end : Intrinsic::invariant_end;
 
       auto Args = F->getFunctionType()->params();
       Type* ObjectPtr[1] = {Args[IsLifetimeEnd ? 1 : 2]};
-      if (F->getName() != Intrinsic::getName(ID, ObjectPtr)) {
+      if (F->getName() !=
+          Intrinsic::getName(ID, ObjectPtr, F->getParent(), nullptr)) {
         rename(F);
         NewFn = Intrinsic::getDeclaration(F->getParent(), ID, ObjectPtr);
         return true;
       }
     }
     if (Name.startswith("invariant.group.barrier")) {
       // Rename invariant.group.barrier to launder.invariant.group
       auto Args = F->getFunctionType()->params();
case 'n': {
     break;
   }
   case 'o':
     // We only need to change the name to match the mangling including the
     // address space.
     if (Name.startswith("objectsize.")) {
       Type *Tys[2] = { F->getReturnType(), F->arg_begin()->getType() };
       if (F->arg_size() == 2 || F->arg_size() == 3 ||
-          F->getName() != Intrinsic::getName(Intrinsic::objectsize, Tys)) {
+          F->getName() != Intrinsic::getName(Intrinsic::objectsize, Tys,
+                                             F->getParent(), nullptr)) {
         rename(F);
         NewFn = Intrinsic::getDeclaration(F->getParent(), Intrinsic::objectsize,
                                           Tys);
         return true;
       }
     }
     break;
 

llvm/test/Assembler/auto_upgrade_intrinsics.ll

 ; Test to make sure intrinsics are automatically upgraded.
 ; RUN: llvm-as < %s | llvm-dis | FileCheck %s
 ; RUN: verify-uselistorder %s
 
+%0 = type opaque;
+
 declare i8 @llvm.ctlz.i8(i8)
 declare i16 @llvm.ctlz.i16(i16)
 declare i32 @llvm.ctlz.i32(i32)
 declare i42 @llvm.ctlz.i42(i42)  ; Not a power-of-2
 
 
-declare i32 @llvm.objectsize.i32(i8*, i1) nounwind readonly
-
-
 define void @test.ctlz(i8 %a, i16 %b, i32 %c, i42 %d) {
 ; CHECK: @test.ctlz
 
 entry:
   ; CHECK: call i8 @llvm.ctlz.i8(i8 %a, i1 false)
   call i8 @llvm.ctlz.i8(i8 %a)
   ; CHECK: call i16 @llvm.ctlz.i16(i16 %b, i1 false)
   call i16 @llvm.ctlz.i16(i16 %b)
entry:
   call i42 @llvm.cttz.i42(i42 %d)
 
   ret void
 }
 
 
 @a = private global [60 x i8] zeroinitializer, align 1
 
+declare i32 @llvm.objectsize.i32(i8*, i1) nounwind readonly
 define i32 @test.objectsize() {
 ; CHECK-LABEL: @test.objectsize(
 ; CHECK: @llvm.objectsize.i32.p0i8(i8* getelementptr inbounds ([60 x i8], [60 x i8]* @a, i32 0, i32 0), i1 false, i1 false, i1 false)
   %s = call i32 @llvm.objectsize.i32(i8* getelementptr inbounds ([60 x i8], [60 x i8]* @a, i32 0, i32 0), i1 false)
   ret i32 %s
 }
 
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1) nounwind readonly
 define i64 @test.objectsize.2() {
 ; CHECK-LABEL: @test.objectsize.2(
 ; CHECK: @llvm.objectsize.i64.p0i8(i8* getelementptr inbounds ([60 x i8], [60 x i8]* @a, i32 0, i32 0), i1 false, i1 false, i1 false)
   %s = call i64 @llvm.objectsize.i64.p0i8(i8* getelementptr inbounds ([60 x i8], [60 x i8]* @a, i32 0, i32 0), i1 false)
   ret i64 %s
 }
 
+@u = private global [60 x %0*] zeroinitializer, align 1
+
+declare i32 @llvm.objectsize.i32.unnamed(%0**, i1) nounwind readonly
+define i32 @test.objectsize.unnamed() {
+; CHECK-LABEL: @test.objectsize.unnamed(
+; CHECK: @llvm.objectsize.i32.p0p0s_s.0(%0** getelementptr inbounds ([60 x %0*], [60 x %0*]* @u, i32 0, i32 0), i1 false, i1 false, i1 false)
+  %s = call i32 @llvm.objectsize.i32.unnamed(%0** getelementptr inbounds ([60 x %0*], [60 x %0*]* @u, i32 0, i32 0), i1 false)
+  ret i32 %s
+}
+
+declare i64 @llvm.objectsize.i64.p0p0s_s.0(%0**, i1) nounwind readonly
+define i64 @test.objectsize.unnamed.2() {
+; CHECK-LABEL: @test.objectsize.unnamed.2(
+; CHECK: @llvm.objectsize.i64.p0p0s_s.0(%0** getelementptr inbounds ([60 x %0*], [60 x %0*]* @u, i32 0, i32 0), i1 false, i1 false, i1 false)
+  %s = call i64 @llvm.objectsize.i64.p0p0s_s.0(%0** getelementptr inbounds ([60 x %0*], [60 x %0*]* @u, i32 0, i32 0), i1 false)
+  ret i64 %s
+}
+
 declare <2 x double> @llvm.masked.load.v2f64(<2 x double>* %ptrs, i32, <2 x i1> %mask, <2 x double> %src0)
 
 define <2 x double> @tests.masked.load(<2 x double>* %ptr, <2 x i1> %mask, <2 x double> %passthru)  {
 ; CHECK-LABEL: @tests.masked.load(
 ; CHECK: @llvm.masked.load.v2f64.p0v2f64
   %res = call <2 x double> @llvm.masked.load.v2f64(<2 x double>* %ptr, i32 1, <2 x i1> %mask, <2 x double> %passthru)
   ret <2 x double> %res
 }
define void @tests.invariant.start.end() {
   %i = call {}* @llvm.invariant.start(i64 1, i8* %a)
   ; CHECK: call {}* @llvm.invariant.start.p0i8
   store i8 0, i8* %a
   call void @llvm.invariant.end({}* %i, i64 1, i8* %a)
   ; CHECK: call void @llvm.invariant.end.p0i8
   ret void
 }
 
+declare {}* @llvm.invariant.start.unnamed(i64, %0** nocapture) nounwind readonly
+declare void @llvm.invariant.end.unnamed({}*, i64, %0** nocapture) nounwind
+
+define void @tests.invariant.start.end.unnamed() {
+  ; CHECK-LABEL: @tests.invariant.start.end.unnamed(
+  %a = alloca %0*
+  %i = call {}* @llvm.invariant.start.unnamed(i64 1, %0** %a)
+  ; CHECK: call {}* @llvm.invariant.start.p0p0s_s.0
+  store %0* null, %0** %a
+  call void @llvm.invariant.end.unnamed({}* %i, i64 1, %0** %a)
+  ; CHECK: call void @llvm.invariant.end.p0p0s_s.0
+  ret void
+}
+
 @__stack_chk_guard = external global i8*
 declare void @llvm.stackprotectorcheck(i8**)
 
 define void @test.stackprotectorcheck() {
 ; CHECK-LABEL: @test.stackprotectorcheck(
 ; CHECK-NEXT: ret void
   call void @llvm.stackprotectorcheck(i8** @__stack_chk_guard)
   ret void
 }
 
 declare void  @llvm.lifetime.start(i64, i8* nocapture) nounwind readonly
 declare void @llvm.lifetime.end(i64, i8* nocapture) nounwind
 
 define void @tests.lifetime.start.end() {
   ; CHECK-LABEL: @tests.lifetime.start.end(
   %a = alloca i8
   call void @llvm.lifetime.start(i64 1, i8* %a)
   ; CHECK: call void @llvm.lifetime.start.p0i8(i64 1, i8* %a)
   store i8 0, i8* %a
   call void @llvm.lifetime.end(i64 1, i8* %a)
   ; CHECK: call void @llvm.lifetime.end.p0i8(i64 1, i8* %a)
   ret void
 }
 
+declare void  @llvm.lifetime.start.unnamed(i64, %0** nocapture) nounwind readonly
+declare void @llvm.lifetime.end.unnamed(i64, %0** nocapture) nounwind
+
+define void @tests.lifetime.start.end.unnamed() {
+  ; CHECK-LABEL: @tests.lifetime.start.end.unnamed(
+  %a = alloca %0*
+  call void @llvm.lifetime.start.unnamed(i64 1, %0** %a)
+  ; CHECK: call void @llvm.lifetime.start.p0p0s_s.0(i64 1, %0** %a)
+  store %0* null, %0** %a
+  call void @llvm.lifetime.end.unnamed(i64 1, %0** %a)
+  ; CHECK: call void @llvm.lifetime.end.p0p0s_s.0(i64 1, %0** %a)
+  ret void
+}
+
 declare void @llvm.prefetch(i8*, i32, i32, i32)
 define void @test.prefetch(i8* %ptr) {
 ; CHECK-LABEL: @test.prefetch(
 ; CHECK: @llvm.prefetch.p0i8(i8* %ptr, i32 0, i32 3, i32 2)
   call void @llvm.prefetch(i8* %ptr, i32 0, i32 3, i32 2)
   ret void
 }
 
 declare void @llvm.prefetch.p0i8(i8*, i32, i32, i32)
 define void @test.prefetch.2(i8* %ptr) {
 ; CHECK-LABEL: @test.prefetch.2(
 ; CHECK: @llvm.prefetch.p0i8(i8* %ptr, i32 0, i32 3, i32 2)
   call void @llvm.prefetch(i8* %ptr, i32 0, i32 3, i32 2)
   ret void
 }
 
 ; This is part of @test.objectsize(), since llvm.objectsize declaration gets
 ; emitted at the end.
 ; CHECK: declare i32 @llvm.objectsize.i32.p0i8
-
+; CHECK: declare i32 @llvm.objectsize.i32.p0p0s_s.0
 
 ; CHECK: declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
 ; CHECK: declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
+; CHECK: declare void @llvm.lifetime.start.p0p0s_s.0(i64 immarg, %0** nocapture)
+; CHECK: declare void @llvm.lifetime.end.p0p0s_s.0(i64 immarg, %0** nocapture)