Don't drop the llvm. prefix when renaming

[• rafael]

Summary

If the llvm. prefix is dropped other parts of llvm don't see this as an intrinsic. This means that the number of regular symbols depends on the context the module is loaded into, which causes LTO to abort.

[apilipenko]

The change seems fine, but I'm curious how we end up with an old renamed intrinsic declaration? For some reason I can't reproduce the problem locally.

If we materialize the whole module we must remove old declarations from the module (see BitcodeReader::materializeModule). Althought if we use BitcodeReader::materialize old declaration will be kept in the module.

[apilipenko]

I reproduced the problem with the testcase from the review. From what I see we lazily materialize the module, so old versions of upgraded intrinsics are not removed.

[• rafael]

283117

lib/IR/AutoUpgrade.cpp

 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Regex.h"
 #include <cstring>
 using namespace llvm;
 
+static void rename(GlobalValue *GV) { GV->setName(GV->getName() + ".old"); }
+
 // Upgrade the declarations of the SSE4.1 functions whose arguments have
 // changed their type from v4f32 to v2i64.
 static bool UpgradeSSE41Function(Function* F, Intrinsic::ID IID,
                                  Function *&NewFn) {
   // Check whether this is an old version of the function, which received
   // v4f32 arguments.
   Type *Arg0Type = F->getFunctionType()->getParamType(0);
   if (Arg0Type != VectorType::get(Type::getFloatTy(F->getContext()), 4))
     return false;
 
   // Yes, it's old, replace it with new version.
-  F->setName(F->getName() + ".old");
+  rename(F);
   NewFn = Intrinsic::getDeclaration(F->getParent(), IID);
   return true;
 }
 
 // Upgrade the declarations of intrinsic functions whose 8-bit immediate mask
 // arguments have changed their type from i32 to i8.
 static bool UpgradeX86IntrinsicsWith8BitMask(Function *F, Intrinsic::ID IID,
                                              Function *&NewFn) {
   // Check that the last argument is an i32.
   Type *LastArgType = F->getFunctionType()->getParamType(
      F->getFunctionType()->getNumParams() - 1);
   if (!LastArgType->isIntegerTy(32))
     return false;
 
   // Move this function aside and map down.
-  F->setName(F->getName() + ".old");
+  rename(F);
   NewFn = Intrinsic::getDeclaration(F->getParent(), IID);
   return true;
 }
 
 static bool UpgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
   assert(F && "Illegal to upgrade a non-existent Function.");
 
   // Quickly eliminate it, if it's not a candidate.
if (Name == "aarch64.thread.pointer" || Name == "arm.thread.pointer") {
       NewFn = Intrinsic::getDeclaration(F->getParent(), Intrinsic::thread_pointer);
       return true;
     }
     break;
   }
 
   case 'c': {
     if (Name.startswith("ctlz.") && F->arg_size() == 1) {
-      F->setName(Name + ".old");
+      rename(F);
       NewFn = Intrinsic::getDeclaration(F->getParent(), Intrinsic::ctlz,
                                         F->arg_begin()->getType());
       return true;
     }
     if (Name.startswith("cttz.") && F->arg_size() == 1) {
-      F->setName(Name + ".old");
+      rename(F);
       NewFn = Intrinsic::getDeclaration(F->getParent(), Intrinsic::cttz,
                                         F->arg_begin()->getType());
       return true;
     }
     break;
   }
   case 'i': {
     if (Name.startswith("invariant.start")) {
       auto Args = F->getFunctionType()->params();
       Type* ObjectPtr[1] = {Args[1]};
       if (F->getName() !=
           Intrinsic::getName(Intrinsic::invariant_start, ObjectPtr)) {
-        F->setName(Name + ".old");
+        rename(F);
         NewFn = Intrinsic::getDeclaration(
             F->getParent(), Intrinsic::invariant_start, ObjectPtr);
         return true;
       }
     }
     if (Name.startswith("invariant.end")) {
       auto Args = F->getFunctionType()->params();
       Type* ObjectPtr[1] = {Args[2]};
       if (F->getName() !=
           Intrinsic::getName(Intrinsic::invariant_end, ObjectPtr)) {
-        F->setName(Name + ".old");
+        rename(F);
         NewFn = Intrinsic::getDeclaration(F->getParent(),
                                           Intrinsic::invariant_end, ObjectPtr);
         return true;
       }
     }
     break;
   }
   case 'm': {
     if (Name.startswith("masked.load.")) {
       Type *Tys[] = { F->getReturnType(), F->arg_begin()->getType() };
       if (F->getName() != Intrinsic::getName(Intrinsic::masked_load, Tys)) {
-        F->setName(Name + ".old");
+        rename(F);
         NewFn = Intrinsic::getDeclaration(F->getParent(),
                                           Intrinsic::masked_load,
                                           Tys);
         return true;
       }
     }
     if (Name.startswith("masked.store.")) {
       auto Args = F->getFunctionType()->params();
       Type *Tys[] = { Args[0], Args[1] };
       if (F->getName() != Intrinsic::getName(Intrinsic::masked_store, Tys)) {
-        F->setName(Name + ".old");
+        rename(F);
         NewFn = Intrinsic::getDeclaration(F->getParent(),
                                           Intrinsic::masked_store,
                                           Tys);
         return true;
       }
     }
     break;
   }
 
   case 'o':
     // We only need to change the name to match the mangling including the
     // address space.
     if (F->arg_size() == 2 && Name.startswith("objectsize.")) {
       Type *Tys[2] = { F->getReturnType(), F->arg_begin()->getType() };
       if (F->getName() != Intrinsic::getName(Intrinsic::objectsize, Tys)) {
-        F->setName(Name + ".old");
+        rename(F);
         NewFn = Intrinsic::getDeclaration(F->getParent(),
                                           Intrinsic::objectsize, Tys);
         return true;
       }
     }
     break;
 
   case 's':
if (IsX86 && Name == "avx.dp.ps.256")
       return UpgradeX86IntrinsicsWith8BitMask(F, Intrinsic::x86_avx_dp_ps_256,
                                               NewFn);
     if (IsX86 && Name == "avx2.mpsadbw")
       return UpgradeX86IntrinsicsWith8BitMask(F, Intrinsic::x86_avx2_mpsadbw,
                                               NewFn);
 
     // frcz.ss/sd may need to have an argument dropped
     if (IsX86 && Name.startswith("xop.vfrcz.ss") && F->arg_size() == 2) {
-      F->setName(Name + ".old");
+      rename(F);
       NewFn = Intrinsic::getDeclaration(F->getParent(),
                                         Intrinsic::x86_xop_vfrcz_ss);
       return true;
     }
     if (IsX86 && Name.startswith("xop.vfrcz.sd") && F->arg_size() == 2) {
-      F->setName(Name + ".old");
+      rename(F);
       NewFn = Intrinsic::getDeclaration(F->getParent(),
                                         Intrinsic::x86_xop_vfrcz_sd);
       return true;
     }
     if (IsX86 && (Name.startswith("avx512.mask.pslli.") ||
                   Name.startswith("avx512.mask.psrai.") ||
                   Name.startswith("avx512.mask.psrli."))) {
       Intrinsic::ID ShiftID;
       if (Name.slice(12, 16) == "psll")
         ShiftID = Name[18] == 'd' ? Intrinsic::x86_avx512_mask_psll_di_512
                                   : Intrinsic::x86_avx512_mask_psll_qi_512;
       else if (Name.slice(12, 16) == "psra")
         ShiftID = Name[18] == 'd' ? Intrinsic::x86_avx512_mask_psra_di_512
                                   : Intrinsic::x86_avx512_mask_psra_qi_512;
       else
         ShiftID = Name[18] == 'd' ? Intrinsic::x86_avx512_mask_psrl_di_512
                                   : Intrinsic::x86_avx512_mask_psrl_qi_512;
-      F->setName("llvm.x86." + Name + ".old");
+      rename(F);
       NewFn = Intrinsic::getDeclaration(F->getParent(), ShiftID);
       return true;
     }
     // Fix the FMA4 intrinsics to remove the 4
     if (IsX86 && Name.startswith("fma4.")) {
-      F->setName("llvm.x86.fma" + Name.substr(5));
+      rename(F);
       NewFn = F;
       return true;
     }
     // Upgrade any XOP PERMIL2 index operand still using a float/double vector.
     if (IsX86 && Name.startswith("xop.vpermil2")) {
       auto Params = F->getFunctionType()->params();
       auto Idx = Params[2];
       if (Idx->getScalarType()->isFloatingPointTy()) {
-        F->setName("llvm.x86." + Name + ".old");
+        rename(F);
         unsigned IdxSize = Idx->getPrimitiveSizeInBits();
         unsigned EltSize = Idx->getScalarSizeInBits();
         Intrinsic::ID Permil2ID;
         if (EltSize == 64 && IdxSize == 128)
           Permil2ID = Intrinsic::x86_xop_vpermil2pd;
         else if (EltSize == 32 && IdxSize == 128)
           Permil2ID = Intrinsic::x86_xop_vpermil2ps;
         else if (EltSize == 64 && IdxSize == 256)

test/LTO/Resolution/X86/Inputs/intrinsic.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+%foo = type {  }
+declare <4 x %foo*> @llvm.masked.load.v4p0foo.p0v4p0foo(<4 x %foo*>*, i32, <4 x i1>, <4 x %foo*>)

test/LTO/Resolution/X86/intrinsic.ll

+; RUN: llvm-as %s -o %t1.o
+; RUN: llvm-as %p/Inputs/intrinsic.ll -o %t2.o
+; RUN: llvm-lto2 -o %t3.o %t1.o %t2.o -r %t1.o,foo
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+%foo = type {  }
+declare void @foo( %foo*  )