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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy 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;
----------------
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).



https://reviews.llvm.org/D30422