Remangle intrinsics names when types are renamed

[apilipenko]

Summary

This is a fix for the problem mentioned in "LTO and intrinsics mangling" llvm-dev mail thread:
http://lists.llvm.org/pipermail/llvm-dev/2016-April/098387.html

Two approaches to fix the problem were mentioned: remangle intrinsics names when types are renamed or avoid using pointer other than i8* in masked load/store intrinsics. I prototyped both and decided to go ahead with the former. It's just cleaner and simple than shuffling masked load/store arguments in autoupgrade/builder/tests. It's also more generic and I expect that we will have the same problem with gather/scatter when we decide to support arbitrary address spaces in these intrinsics.

In this patch I reuse AutoUpgrade to trigger intrinsic renaming in the case when types were renamed during module loading. I also changed IRMover to rename the intrinsics if the types were remapped.

[mehdi_amini]

I have a strong feeling against any auto upgrade of the bitcode produced by the current version of llvm

[mehdi_amini]

Let me clarify: I'm not opposed about doing the renaming during bitcode loading, I just think it is wrong to use the autoupdate path.
I'd expect that it could be done at the time you parse the intrinsic from the bitcode?

[apilipenko]

Potentially renaming can be needed for textual representation as well. So this functionality has to be shared between parsers. I can do something similar to AutoUpgrade for this specific scenario, but it will duplicate AutoUpgrade logic about iterating over declarations/call sites.

[apilipenko]

Don't use AutoUpgrade anymore. I'm going to add tests as soon as we agreed on the functional part.

[mehdi_amini]

lib/AsmParser/LLParser.cpp

223 ↗

(On Diff #54660)

RAUW does not work at this point?

lib/IR/Function.cpp

1050 ↗

(On Diff #54660)

naming

1054 ↗

(On Diff #54660)

Guess you didn't want it here.

1069 ↗

(On Diff #54660)

std::all_of

1079 ↗

(On Diff #54660)

There are two conceptually separated path IIUC:

1. old bitcode the intrinsic is *not* mangled and need to be mangled. This is the auto-upgrade path.
2. current bitcode has named type collision in the context and you need to remangle.

The fact that the remangling is shared is fine. However the conditions to reach this should be asserted better: the first path is valid only if the intrinsic was not mangled in the first place. The second path is valid only if the mismatch is because of a suffix added to the type.

[apilipenko]

lib/AsmParser/LLParser.cpp

223 ↗

(On Diff #54660)

It works, autoupgrade uses it, but what are the benefits over changing the existing call instruction?

lib/IR/Function.cpp

1079 ↗

(On Diff #54660)

In the first "not mangled intrinsic" path do you mean masked load/store intrinsics specifically?

In general, first I try to upgrade an intrinsic (see BitcodeReader.cpp:3220). If it was upgraded I assume that the name and the signature matches and no further remangling is needed. If it wasn't upgraded I check if it needs remangling. We can only remangle intrinsics which signature matches with the .td descriptor, that's why I do upgrade first.

With masked load/store autoupgrade patch applied in case of renamed types autoupgrade bit kicks in first and remangles the intrinsic. It happens because autoupgrade path also matches the name with the expected name for the given signature. It makes this case slightly convoluted but I don't see any other simple way to check if autoupgrade is needed.

Since we know the old mangling scheme for the masked intrinsics we might try to match the name with the old mangling name.

if (Name == Intrinsics::getName(masked_load, OldTys)
  // Upgrade

But it won't work in case when types were renamed.

Difficulties separating upgrade and remangling paths was one of the reasons I combined them both in autoupgrade in the initial revision.

[reames]

Minor comments. Note that there are open questions here which I'm not qualified to answer. joker.eph?

lib/AsmParser/LLParser.cpp

222 ↗

(On Diff #54660)

You can also invoke some intrinsics. Use CallSite or handle InvokeInst.

223 ↗

(On Diff #54660)

RAUW is idiomtic. Please use it unless there's a reason not to.

lib/Bitcode/Reader/BitcodeReader.cpp

3222 ↗

(On Diff #54660)

Could this be pushed inside upgradeIntrinsicFunction?

5381 ↗

(On Diff #54660)

Overly complicated. Can this be expressed as a single loop which does a then b?

lib/IR/Function.cpp

1069 ↗

(On Diff #54660)

or for (auto *P : FTy->params())

[apilipenko]

@reames, your suggestion to move the logic to upgradeIntrinsicFunction is basically what I had in the first diff of the review. But there was an argument to avoid using autoupgarde for cases which are technically not upgrade, i.e. for the IR generated by the same version of LLVM.

lib/AsmParser/LLParser.cpp

222 ↗

(On Diff #54660)

Good catch, will fix.

[apilipenko]

Take some of the Philip's comments into account: add handling for invokes and use for-each loop in remangleIntrinsicFunction.

[apilipenko]

Ping.

[mehdi_amini]

Tests for all theses cases?

lib/AsmParser/LLParser.cpp

224 ↗

(On Diff #58615)

RAUW?

lib/Bitcode/Reader/BitcodeReader.cpp

5587 ↗

(On Diff #58615)

Why do we update only calls and not all uses?
If we don't expect any other use of an intrinsic than calls, then we should just cast.

lib/IR/Function.cpp

1050 ↗

(On Diff #58615)

ping (upper case for variable)

[apilipenko]

Address review comments, tests added.

[mehdi_amini]

LGTM, thks.

llvm/trunk/include/llvm/IR/Intrinsics.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_INTRINSICS_H
 #define LLVM_IR_INTRINSICS_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include <string>
 
 namespace llvm {
 
 class Type;
 class FunctionType;
 class Function;
 class LLVMContext;
#undef GET_INTRINSIC_ENUM_VALUES
   /// value) with the type constraints specified by the .td file. If the given
   /// type is an overloaded type it is pushed to the ArgTys vector.
   ///
   /// Returns false if the given type matches with the constraints, true
   /// otherwise.
   bool matchIntrinsicType(Type *Ty, ArrayRef<IITDescriptor> &Infos,
                           SmallVectorImpl<Type*> &ArgTys);
 
+  // Checks if the intrinsic name matches with its signature and if not
+  // returns the declaration with the same signature and remangled name.
+  llvm::Optional<Function*> remangleIntrinsicFunction(Function *F);
+
 } // End Intrinsic namespace
 
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/AsmParser/LLParser.cpp

 
 #include "LLParser.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/AsmParser/SlotMapping.h"
 #include "llvm/IR/AutoUpgrade.h"
 #include "llvm/IR/CallingConv.h"
+#include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/ValueSymbolTable.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Dwarf.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/SaveAndRestore.h"
bool LLParser::ValidateEndOfModule() {
 
   for (unsigned I = 0, E = InstsWithTBAATag.size(); I < E; I++)
     UpgradeInstWithTBAATag(InstsWithTBAATag[I]);
 
   // Look for intrinsic functions and CallInst that need to be upgraded
   for (Module::iterator FI = M->begin(), FE = M->end(); FI != FE; )
     UpgradeCallsToIntrinsic(&*FI++); // must be post-increment, as we remove
 
+  // Some types could be renamed during loading if several modules are
+  // loaded in the same LLVMContext (LTO scenario). In this case we should
+  // remangle intrinsics names as well.
+  for (Module::iterator FI = M->begin(), FE = M->end(); FI != FE; ) {
+    Function *F = &*FI++;
+    if (auto Remangled = Intrinsic::remangleIntrinsicFunction(F)) {
+      F->replaceAllUsesWith(Remangled.getValue());
+      F->eraseFromParent();
+    }
+  }
+
   UpgradeDebugInfo(*M);
 
   UpgradeModuleFlags(*M);
 
   if (!Slots)
     return false;
   // Initialize the slot mapping.
   // Because by this point we've parsed and validated everything, we can "steal"

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Bitcode/BitstreamReader.h"
 #include "llvm/Bitcode/LLVMBitCodes.h"
 #include "llvm/Bitcode/ReaderWriter.h"
 #include "llvm/IR/AutoUpgrade.h"
+#include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/GVMaterializer.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/IntrinsicInst.h"
class BitcodeReader : public GVMaterializer {
   std::vector<BasicBlock*> FunctionBBs;
 
   // When reading the module header, this list is populated with functions that
   // have bodies later in the file.
   std::vector<Function*> FunctionsWithBodies;
 
   // When intrinsic functions are encountered which require upgrading they are
   // stored here with their replacement function.
-  typedef DenseMap<Function*, Function*> UpgradedIntrinsicMap;
-  UpgradedIntrinsicMap UpgradedIntrinsics;
+  typedef DenseMap<Function*, Function*> UpdatedIntrinsicMap;
+  UpdatedIntrinsicMap UpgradedIntrinsics;
+  // Intrinsics which were remangled because of types rename
+  UpdatedIntrinsicMap RemangledIntrinsics;
 
   // Map the bitcode's custom MDKind ID to the Module's MDKind ID.
   DenseMap<unsigned, unsigned> MDKindMap;
 
   // Several operations happen after the module header has been read, but
   // before function bodies are processed. This keeps track of whether
   // we've done this yet.
   bool SeenFirstFunctionBody = false;
std::error_code BitcodeReader::globalCleanup() {
   if (!GlobalInits.empty() || !IndirectSymbolInits.empty())
     return error("Malformed global initializer set");
 
   // Look for intrinsic functions which need to be upgraded at some point
   for (Function &F : *TheModule) {
     Function *NewFn;
     if (UpgradeIntrinsicFunction(&F, NewFn))
       UpgradedIntrinsics[&F] = NewFn;
+    else if (auto Remangled = Intrinsic::remangleIntrinsicFunction(&F))
+      // Some types could be renamed during loading if several modules are
+      // loaded in the same LLVMContext (LTO scenario). In this case we should
+      // remangle intrinsics names as well.
+      RemangledIntrinsics[&F] = Remangled.getValue();
   }
 
   // Look for global variables which need to be renamed.
   for (GlobalVariable &GV : TheModule->globals())
     UpgradeGlobalVariable(&GV);
 
   // Force deallocation of memory for these vectors to favor the client that
   // want lazy deserialization.
for (auto UI = I.first->materialized_user_begin(), UE = I.first->user_end();
          UI != UE;) {
       User *U = *UI;
       ++UI;
       if (CallInst *CI = dyn_cast<CallInst>(U))
         UpgradeIntrinsicCall(CI, I.second);
     }
   }
 
+  // Update calls to the remangled intrinsics
+  for (auto &I : RemangledIntrinsics)
+    for (auto UI = I.first->materialized_user_begin(), UE = I.first->user_end();
+         UI != UE;)
+      // Don't expect any other users than call sites
+      CallSite(*UI++).setCalledFunction(I.second);
+
   // Finish fn->subprogram upgrade for materialized functions.
   if (DISubprogram *SP = FunctionsWithSPs.lookup(F))
     F->setSubprogram(SP);
 
   // Bring in any functions that this function forward-referenced via
   // blockaddresses.
   return materializeForwardReferencedFunctions();
 }
for (auto *U : I.first->users()) {
       if (CallInst *CI = dyn_cast<CallInst>(U))
         UpgradeIntrinsicCall(CI, I.second);
     }
     if (!I.first->use_empty())
       I.first->replaceAllUsesWith(I.second);
     I.first->eraseFromParent();
   }
   UpgradedIntrinsics.clear();
+  // Do the same for remangled intrinsics
+  for (auto &I : RemangledIntrinsics) {
+    I.first->replaceAllUsesWith(I.second);
+    I.first->eraseFromParent();
+  }
+  RemangledIntrinsics.clear();
 
   UpgradeDebugInfo(*TheModule);
 
   UpgradeModuleFlags(*TheModule);
   return std::error_code();
 }
 
 std::vector<StructType *> BitcodeReader::getIdentifiedStructTypes() const {

llvm/trunk/lib/IR/Function.cpp

case IITDescriptor::VecOfPtrsToElt: {
         return true;
       return ThisArgEltTy->getElementType() !=
              ReferenceType->getVectorElementType();
     }
   }
   llvm_unreachable("unhandled");
 }
 
+Optional<Function*> Intrinsic::remangleIntrinsicFunction(Function *F) {
+  Intrinsic::ID ID = F->getIntrinsicID();
+  if (!ID)
+    return None;
+
+  // Accumulate an array of overloaded types for the given intrinsic
+  SmallVector<Type *, 4> ArgTys;
+  {
+    SmallVector<Intrinsic::IITDescriptor, 8> Table;
+    getIntrinsicInfoTableEntries(ID, Table);
+    ArrayRef<Intrinsic::IITDescriptor> TableRef = Table;
+    FunctionType *FTy = F->getFunctionType();
+
+    // If we encounter any problems matching the signature with the descriptor
+    // just give up remangling. It's up to verifier to report the discrepancy.
+    if (Intrinsic::matchIntrinsicType(FTy->getReturnType(), TableRef, ArgTys))
+      return None;
+    for (auto Ty : FTy->params())
+      if (Intrinsic::matchIntrinsicType(Ty, TableRef, ArgTys))
+        return None;
+  }
+
+  StringRef Name = F->getName();
+  if (Name == Intrinsic::getName(ID, ArgTys))
+    return None;
+
+  auto NewDecl = Intrinsic::getDeclaration(F->getParent(), ID, ArgTys);
+  NewDecl->setCallingConv(F->getCallingConv());
+  return NewDecl;
+}
+
 /// hasAddressTaken - returns true if there are any uses of this function
 /// other than direct calls or invokes to it.
 bool Function::hasAddressTaken(const User* *PutOffender) const {
   for (const Use &U : uses()) {
     const User *FU = U.getUser();
     if (isa<BlockAddress>(FU))
       continue;
     if (!isa<CallInst>(FU) && !isa<InvokeInst>(FU)) {

llvm/trunk/lib/Linker/IRMover.cpp

 #include "LinkDiagnosticInfo.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/GVMaterializer.h"
+#include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/TypeFinder.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include <utility>
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 // TypeMap implementation.
if (DGV && !ShouldLink) {
     // reference, simply map it to null.
     if (DoneLinkingBodies)
       return nullptr;
 
     NewGV = copyGlobalValueProto(SGV, ShouldLink);
     if (ShouldLink || !ForAlias)
       forceRenaming(NewGV, SGV->getName());
   }
+
+  // Overloaded intrinsics have overloaded types names as part of their
+  // names. If we renamed overloaded types we should rename the intrinsic
+  // as well.
+  if (Function *F = dyn_cast<Function>(NewGV))
+    if (auto Remangled = Intrinsic::remangleIntrinsicFunction(F))
+      NewGV = Remangled.getValue();
+
   if (ShouldLink || ForAlias) {
     if (const Comdat *SC = SGV->getComdat()) {
       if (auto *GO = dyn_cast<GlobalObject>(NewGV)) {
         Comdat *DC = DstM.getOrInsertComdat(SC->getName());
         DC->setSelectionKind(SC->getSelectionKind());
         GO->setComdat(DC);
       }
     }

llvm/trunk/test/LTO/X86/Inputs/remangle_intrinsics.ll

+%struct.rtx_def = type { i16, i16 }
+
+define void @bar(%struct.rtx_def* %a, i8 %b, i32 %c) {
+  call void  @llvm.memset.p0struct.rtx_def.i32(%struct.rtx_def* %a, i8 %b, i32 %c, i32 4, i1 true)
+  ret void
+}
+
+declare void @llvm.memset.p0struct.rtx_def.i32(%struct.rtx_def*, i8, i32, i32, i1)

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

+; RUN: llvm-as < %s > %t1
+; RUN: llvm-as < %p/Inputs/remangle_intrinsics.ll > %t2
+; RUN: llvm-lto %t1 %t2 | FileCheck %s
+
+; We have "struct.rtx_def" type in both modules being LTOed. Both modules use
+; an overloaded intrinsic which has this type in its signature/name. When
+; modules are loaded one of the types is renamed to "struct.rtx_def.0".
+; The intrinsic which uses this type should be remangled/renamed as well.
+; If we didn't do that verifier would complain.
+
+; CHECK: Wrote native object file
+
+%struct.rtx_def = type { i16 }
+
+define void @foo(%struct.rtx_def* %a, i8 %b, i32 %c) {
+  call void  @llvm.memset.p0struct.rtx_def.i32(%struct.rtx_def* %a, i8 %b, i32 %c, i32 4, i1 true)
+  ret void
+}
+
+declare void @llvm.memset.p0struct.rtx_def.i32(%struct.rtx_def*, i8, i32, i32, i1)

llvm/trunk/unittests/Linker/LinkModulesTest.cpp

TEST_F(LinkModuleTest, MoveDistinctMDs) {
   EXPECT_TRUE(M0->isDistinct());
   EXPECT_TRUE(M1->isDistinct());
   EXPECT_TRUE(M2->isDistinct());
   EXPECT_TRUE(M3->isDistinct());
   EXPECT_TRUE(M4->isUniqued());
   EXPECT_EQ(M3, M4->getOperand(0));
 }
 
+TEST_F(LinkModuleTest, RemangleIntrinsics) {
+  LLVMContext C;
+  SMDiagnostic Err;
+
+  // 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"
+    "  ret void\n"
+    "}\n"
+    "declare void @llvm.memset.p0struct.rtx_def.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"
+    "  ret void\n"
+    "}\n"
+    "declare void @llvm.memset.p0struct.rtx_def.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"));
+
+  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"));
+
+  // 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");
+  ASSERT_EQ(F->getNumUses(), (unsigned)2);
+}
+
 } // end anonymous namespace