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

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

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

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

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

ping (upper case for variable)

[apilipenko]

Address review comments, tests added.

[mehdi_amini]

LGTM, thks.

include/llvm/IR/Intrinsics.h

#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);
 
+  /// Accumulates overloaded types in ArgTys for the given function type and
+  /// intrinsic id.
+  void getOverloadedTypes(FunctionType *FTy, ID id,
+                          SmallVectorImpl<Type*> &ArgTys);
+
 } // End Intrinsic namespace
 
 } // End llvm namespace
 
 #endif

lib/IR/AutoUpgrade.cpp

static bool UpgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
   return false;
 }
 
 bool llvm::UpgradeIntrinsicFunction(Function *F, Function *&NewFn) {
   NewFn = nullptr;
   bool Upgraded = UpgradeIntrinsicFunction1(F, NewFn);
   assert(F != NewFn && "Intrinsic function upgraded to the same function");
 
+  if (!Upgraded)
+    if (Intrinsic::ID ID = F->getIntrinsicID()) {
+      // 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.
+      SmallVector<Type *, 4> Tys;
+      Intrinsic::getOverloadedTypes(F->getFunctionType(), ID, Tys);
+      StringRef Name = F->getName();
+      if (Name != Intrinsic::getName(ID, Tys)) {
+        F->setName(Name + ".old");
+        NewFn = Intrinsic::getDeclaration(F->getParent(), ID, Tys);
+        Upgraded = true;
+      }
+    }
+
   // Upgrade intrinsic attributes.  This does not change the function.
   if (NewFn)
     F = NewFn;
   if (Intrinsic::ID id = F->getIntrinsicID())
     F->setAttributes(Intrinsic::getAttributes(F->getContext(), id));
   return Upgraded;
 }
 
if (!NewFn) {
     return;
   }
 
   std::string Name = CI->getName();
   if (!Name.empty())
     CI->setName(Name + ".old");
 
   switch (NewFn->getIntrinsicID()) {
-  default:
-    llvm_unreachable("Unknown function for CallInst upgrade.");
+  default: {
+    SmallVector<Value *, 8> Args(CI->arg_operands().begin(),
+                                 CI->arg_operands().end());
+    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, Args, Name));
+    CI->eraseFromParent();
+    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:

lib/IR/Function.cpp

case IITDescriptor::VecOfPtrsToElt: {
         return true;
       return ThisArgEltTy->getElementType() !=
              ReferenceType->getVectorElementType();
     }
   }
   llvm_unreachable("unhandled");
 }
 
+void Intrinsic::getOverloadedTypes(FunctionType *FTy, ID id,
+                                   SmallVectorImpl<Type*> &ArgTys) {
+  SmallVector<Intrinsic::IITDescriptor, 8> Table;
+  getIntrinsicInfoTableEntries(id, Table);
+  ArrayRef<Intrinsic::IITDescriptor> TableRef = Table;
+
+  bool Failed = Intrinsic::matchIntrinsicType(FTy->getReturnType(),
+                                              TableRef, ArgTys);
+  assert(!Failed && "Intrinsic return type mismatch");
+  for (unsigned i = 0, e = FTy->getNumParams(); i != e; ++i) {
+    Failed = Intrinsic::matchIntrinsicType(FTy->getParamType(i),
+                                           TableRef, ArgTys);
+    assert(!Failed && "Intrinsic argument type mismatch");
+  }
+}
+
 /// 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;
       return true;
     }
     ImmutableCallSite CS(cast<Instruction>(FU));
     if (!CS.isCallee(&U)) {
       if (PutOffender)
         *PutOffender = FU;
       return true;
     }
   }
   return false;
 }
 
 bool Function::isDefTriviallyDead() const {

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());
   }
+
+  if (Function *F = dyn_cast<Function>(NewGV))
+    if (Intrinsic::ID id = F->getIntrinsicID()) {
+      // Overloaded intrinsics have overloaded types names as part of their
+      // names. If we renamed overloaded types we should rename the intrinsic
+      // as well.
+      SmallVector<Type *, 4> Tys;
+      Intrinsic::getOverloadedTypes(F->getFunctionType(), id, Tys);
+      if (F->getName() != Intrinsic::getName(id, Tys))
+        NewGV = Intrinsic::getDeclaration(&DstM, id, Tys);
+    }
+
   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);
       }
     }