[PATCH] D30422: Keep attributes, calling convention, etc, when remangling intrinsic

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/IR/AutoUpgrade.cpp:2008
     CallInst *NewCall = Builder.CreateCall(NewFn, {BC0, BC1}, Name);
-    CI->replaceAllUsesWith(NewCall);
-    CI->eraseFromParent();
-    return;
+    Repl = NewCall;
+    break;
----------------
sanjoy wrote:
> These should just be `Repl = Builder.CreateCall(NewFn, {BC0, BC1}, Name);`
Yeah, and the break above should be a return.
Looks like my refactoring tool screwed it up.



================
Comment at: lib/IR/AutoUpgrade.cpp:2048
+    if (!Name.empty())
+      CI->setName(Name + ".old");
+    CI->replaceAllUsesWith(Repl);
----------------
sanjoy wrote:
> I'm not sure if this does what used to happen before.  To get the behavior intended by https://reviews.llvm.org/rL146357 I think `CI->setName(Name + ".old");` needs to happen //before// you create a new call instruction with `Name` as name.  Either that, or some shuffling as:
> 
> ```
> if (Name not empty) {
>   CI->setName(Name + ".old");
>   Repl->setName(Name); // This one should "stick"
>   ...
> }
> ```
> 
I don't see a way to set the name beforehand, except either unconditionally, or to do so inside the switches.
The second we could do.
Do we really still believe this is all worth it?

Your original suggestion was:

"if (NewCI != CI) {
  // Adjust names, and kill CI
}
(and remove the pre-rename)?"

That won't work here, as discussed :)
Do we think this is all better than just removing the renaming and letting it all work itself out?
(or always renaming)



https://reviews.llvm.org/D30422