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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 3 inline comments as done.
dberlin added inline comments.


================
Comment at: lib/IR/AutoUpgrade.cpp:1924
         "Unknown function for CallInst upgrade and isn't just a name change");
-    SmallVector<Value *, 4> Args(CI->arg_operands().begin(),
-                                 CI->arg_operands().end());
-    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, Args));
-    CI->eraseFromParent();
+    CI->setCalledFunction(NewFn);
     return;
----------------
sanjoy wrote:
> dberlin wrote:
> > sanjoy wrote:
> > > I think we also need to undo the name change.  Setting it and then unsetting it seems a bit hacky, perhaps we could have the switch create an unnamed `NewCI`, and outside it do:
> > > 
> > > ```
> > > if (NewCI != CI) {
> > >   // Adjust names, and kill CI
> > > }
> > > ```
> > > 
> > > (and remove the pre-rename)?
> > Just out of curiosity, why?
> > I mean, it renames it to old, but who cares?
> > 
> > It is just the name of the call instruction, not the actual called function?
> > 
> > IE it's just changing
> > 
> > %1 = call foo
> > into
> > %1.old = call foo
> > 
> > 
> > We could call it ".upgraded" instead?
> > :)
> > 
> > 
> > Actually, looking closer, all the other paths erase CI completely or leave it alone, so i'm not sure why both changing the name at all.
> > It looks like we are just wasting time?
> > 
> > IE all the paths before it depend on !NewFn and then destroy it, even if there is no replacement. 
> > So if we get to the renaming, we've done nothing to it.
> > 
> > All the paths after either do nothing, erase CI completely, or are the one we are talking about.
> > 
> > Thus it seems we could just leave the name alone and declare victory?
> > 
> > Or am i missing something?
> > 
> I believe that if you call `setName` on the new instruction without changing the name of the old one, since two instructions with the same name are not allowed, the name it actually gets is `name.0` or something like that (see `ValueSymbolTable::createValueName`).  I think renaming the old instruction is a trick to prevent this from happening by avoiding the name collision.
> 
> I personally do not think it is important to avoid names like `name.0` when auto-upgrading; but it looks like it was important enough for someone to add that code (assuming I read the intent correctly).
> 
So, it looks like the code used to rename it to .old in exactly one case.

In that case, it also used a dangling stringref (it's now a std::string copy, as you can see).

As a fix, the renaming code got hoisted, and now everything is renamed
anyway, i did what you suggested in the first case



https://reviews.llvm.org/D30422