[PATCH] D19373: Remangle intrinsics names when types are renamed

[Philip Reames via llvm-commits]

reames added a comment.

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


================
Comment at: lib/AsmParser/LLParser.cpp:222
@@ +221,3 @@
+      for (User *U : F->users())
+        if (CallInst *CI = dyn_cast<CallInst>(U))
+          CI->setCalledFunction(Remangled.getValue());
----------------
You can also invoke some intrinsics.  Use CallSite or handle InvokeInst.

================
Comment at: lib/AsmParser/LLParser.cpp:223
@@ +222,3 @@
+        if (CallInst *CI = dyn_cast<CallInst>(U))
+          CI->setCalledFunction(Remangled.getValue());
+      F->eraseFromParent();
----------------
apilipenko wrote:
> joker.eph wrote:
> > RAUW does not work at this point?
> It works, autoupgrade uses it, but what are the benefits over changing the existing call instruction?
RAUW is idiomtic.  Please use it unless there's a reason not to.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3222
@@ -3219,1 +3221,3 @@
       UpgradedIntrinsics[&F] = NewFn;
+    else if (auto Remangled = Intrinsic::remangleIntrinsicFunction(&F))
+      // Some types could be renamed during loading if several modules are
----------------
Could this be pushed inside upgradeIntrinsicFunction?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5381
@@ +5380,3 @@
+  // Update intrinsics calls in the function
+  auto UpdateIntrinsicsCalls = [] (UpdatedIntrinsicMap IntrinsicsMap,
+                                   void (*Do)(CallInst *, Function *)) {
----------------
Overly complicated.  Can this be expressed as a single loop which does a then b?

================
Comment at: lib/IR/Function.cpp:1069
@@ +1068,3 @@
+    for (unsigned i = 0, e = FTy->getNumParams(); i != e; ++i)
+      if (Intrinsic::matchIntrinsicType(FTy->getParamType(i), TableRef, ArgTys))
+        return None;
----------------
joker.eph wrote:
> std::all_of
or for (auto *P : FTy->params())


http://reviews.llvm.org/D19373