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

224

RAUW does not work at this point?

lib/IR/Function.cpp

1050

naming

1054

Guess you didn't want it here.

1069

std::all_of

1079

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

224

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

lib/IR/Function.cpp

1079

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

223

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

224

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

lib/Bitcode/Reader/BitcodeReader.cpp

3419

Could this be pushed inside upgradeIntrinsicFunction?

5579

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

lib/IR/Function.cpp

1069

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

223

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

RAUW?

lib/Bitcode/Reader/BitcodeReader.cpp

5587

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

ping (upper case for variable)

[apilipenko]

Address review comments, tests added.

[mehdi_amini]

LGTM, thks.

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
   /// Verify that the specified type (which comes from an intrinsic argument or
   /// return value) matches the type constraints specified by the .td file (e.g.
   /// an "any integer" argument really is an integer). Returns true on error.
   ///
   /// If the given type is an overloaded type it is pushed to the ArgTys vector.
   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

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)) {
+      // Replace all users of the old function with the new function
+      for (User *U : F->users())
+        if (CallSite CS = CallSite(U))
+          CS.setCalledFunction(Remangled.getValue());
+      F->eraseFromParent();
+    }
+  }
+
   UpgradeDebugInfo(*M);
 
   if (!Slots)
     return false;
   // Initialize the slot mapping.
   // Because by this point we've parsed and validated everything, we can "steal"
   // the mapping from LLParser as it doesn't need it anymore.
   Slots->GlobalValues = std::move(NumberedVals);

lib/Bitcode/Reader/BitcodeReader.cpp

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.
std::error_code BitcodeReader::materialize(GlobalValue *GV) {
 
   if (std::error_code EC = parseFunctionBody(F))
     return EC;
   F->setIsMaterializable(false);
 
   if (StripDebugInfo)
     stripDebugInfo(*F);
 
-  // Upgrade any old intrinsic calls in the function.
-  for (auto &I : UpgradedIntrinsics) {
-    for (auto UI = I.first->materialized_user_begin(), UE = I.first->user_end();
-         UI != UE;) {
+  // Update intrinsics calls in the function
+  auto UpdateIntrinsicsCalls = [] (UpdatedIntrinsicMap IntrinsicsMap,
+                                   void (*Do)(CallInst *, Function *)) {
+    for (auto &I : IntrinsicsMap) {
+      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);
+          Do(CI, I.second);
       }
     }
+  };
+
+  // Upgrade old intrinsic calls
+  UpdateIntrinsicsCalls(UpgradedIntrinsics, UpgradeIntrinsicCall);
+  // Update calls to the remangled intrinsics
+  UpdateIntrinsicsCalls(RemangledIntrinsics,
+                        [] (CallInst *CI, Function *F) {
+                          CI->setCalledFunction(F);
+                        });
 
   // 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();
std::error_code BitcodeReader::materializeModule() {
   if (!BasicBlockFwdRefs.empty())
     return error("Never resolved function from blockaddress");
 
   // Upgrading intrinsic calls before TBAA can cause TBAA metadata to be lost,
   // to prevent this instructions with TBAA tags should be upgraded first.
   for (unsigned I = 0, E = InstsWithTBAATag.size(); I < E; I++)
     UpgradeInstWithTBAATag(InstsWithTBAATag[I]);
 
-  // Upgrade any intrinsic calls that slipped through (should not happen!) and
+  // Update any intrinsic calls that slipped through (should not happen!) and
   // delete the old functions to clean up. We can't do this unless the entire
   // module is materialized because there could always be another function body
   // with calls to the old function.
-  for (auto &I : UpgradedIntrinsics) {
+  auto UpdateIntrinsicsCalls = [] (UpdatedIntrinsicMap IntrinsicsMap,
+                                   void (*Do)(CallInst *, Function *)) {
+    for (auto &I : IntrinsicsMap) {
       for (auto *U : I.first->users()) {
         if (CallInst *CI = dyn_cast<CallInst>(U))
-        UpgradeIntrinsicCall(CI, I.second);
+          Do(CI, I.second);
       }
       if (!I.first->use_empty())
         I.first->replaceAllUsesWith(I.second);
       I.first->eraseFromParent();
     }
-  UpgradedIntrinsics.clear();
+    IntrinsicsMap.clear();
+  };
+
+  // Upgrade old intrinsics calls
+  UpdateIntrinsicsCalls(UpgradedIntrinsics, UpgradeIntrinsicCall);
+  // Update calls to remangled intrinsics
+  UpdateIntrinsicsCalls(RemangledIntrinsics,
+                        [] (CallInst *CI, Function *F) {
+                          CI->setCalledFunction(F);
+                        });
 
   UpgradeDebugInfo(*TheModule);
   return std::error_code();
 }
 
 std::vector<StructType *> BitcodeReader::getIdentifiedStructTypes() const {
   return IdentifiedStructTypes;
 }

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;
+
+  return Intrinsic::getDeclaration(F->getParent(), id, ArgTys);
+}
+
 /// 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)) {
       if (PutOffender)
         *PutOffender = FU;

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/Transforms/Utils/Cloning.h"
 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);
       }
     }