Keep attributes, calling convention, etc, when remangling intrinsic

[• dberlin]

Summary

Fix issue reported where intrinsic calling convention is dropped after r295253.

[sanjoy]

lib/IR/AutoUpgrade.cpp

1924 ↗

(On Diff #89915)

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)?

[• dberlin]

lib/IR/AutoUpgrade.cpp

1924 ↗

(On Diff #89915)

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?

[sanjoy]

lib/IR/AutoUpgrade.cpp

1924 ↗

(On Diff #89915)

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

[• dberlin]

lib/IR/AutoUpgrade.cpp

1924 ↗

(On Diff #89915)

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

[• dberlin]

• Update for comments

[sanjoy]

lib/IR/AutoUpgrade.cpp

1915 ↗

(On Diff #89960)

How about NewCall instead of Repl?

2008 ↗

(On Diff #89960)

These should just be Repl = Builder.CreateCall(NewFn, {BC0, BC1}, Name);

2048 ↗

(On Diff #89960)

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"
  ...
}

[• dberlin]

lib/IR/AutoUpgrade.cpp

2008 ↗

(On Diff #89960)

Yeah, and the break above should be a return.
Looks like my refactoring tool screwed it up.

2048 ↗

(On Diff #89960)

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)

[• dberlin]

• Update for comments

[sanjoy]

lib/IR/AutoUpgrade.cpp

1920 ↗

(On Diff #90037)

Don't you need a break; here?

1953 ↗

(On Diff #90037)

Spacing is a bit off here.

1958 ↗

(On Diff #90037)

Why did you leave the replaceAllUsesWith alone in this case?

[• dberlin]

lib/IR/AutoUpgrade.cpp

1920 ↗

(On Diff #90037)

No, this should return, sigh.

1953 ↗

(On Diff #90037)

I'll clang format it again

1958 ↗

(On Diff #90037)

Just got missed because it was multiline

[sanjoy]

lib/IR/AutoUpgrade.cpp

1920 ↗

(On Diff #90037)

If you change this to a return then how about keeping NewCall uninitialized (instead of setting it to CI) above and removing the NewCall != CI check below?

[• dberlin]

• Update for comments

[sanjoy]

lgtm with two minor comments inline.

lib/IR/AutoUpgrade.cpp

1955 ↗

(On Diff #90089)

We're doing redundant work here -- I don't think we need to pass in Name to CreateCall, since we're going to set the name explicitly anyway.

Once we've removed these uses of Name, we should be able to sink the std::string Name = CI->getName(); to after the switch.

test/CodeGen/Generic/overloaded-intrinsic-name.ll

2 ↗

(On Diff #90089)

Do we really need a new RUN line here? Why not just tack on a | FileCheck %s to the already existing line?

llvm/trunk/lib/IR/AutoUpgrade.cpp

     }
 
     if (Rep)
       CI->replaceAllUsesWith(Rep);
     CI->eraseFromParent();
     return;
   }
 
-  std::string Name = CI->getName();
-  if (!Name.empty())
-    CI->setName(Name + ".old");
-
+  CallInst *NewCall = nullptr;
   switch (NewFn->getIntrinsicID()) {
   default: {
     // Handle generic mangling change, but nothing else
     assert(
         (CI->getCalledFunction()->getName() != NewFn->getName()) &&
         "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;
   }
 
   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:
   case Intrinsic::arm_neon_vst1:
   case Intrinsic::arm_neon_vst2:
   case Intrinsic::arm_neon_vst3:
   case Intrinsic::arm_neon_vst4:
   case Intrinsic::arm_neon_vst2lane:
   case Intrinsic::arm_neon_vst3lane:
   case Intrinsic::arm_neon_vst4lane: {
     SmallVector<Value *, 4> Args(CI->arg_operands().begin(),
                                  CI->arg_operands().end());
-    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, Args));
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, Args);
+    break;
   }
 
   case Intrinsic::bitreverse:
-    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, {CI->getArgOperand(0)}));
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, {CI->getArgOperand(0)});
+    break;
 
   case Intrinsic::ctlz:
   case Intrinsic::cttz:
     assert(CI->getNumArgOperands() == 1 &&
            "Mismatch between function args and call args");
-    CI->replaceAllUsesWith(Builder.CreateCall(
-        NewFn, {CI->getArgOperand(0), Builder.getFalse()}, Name));
-    CI->eraseFromParent();
-    return;
+    NewCall =
+        Builder.CreateCall(NewFn, {CI->getArgOperand(0), Builder.getFalse()});
+    break;
 
   case Intrinsic::objectsize:
-    CI->replaceAllUsesWith(Builder.CreateCall(
-        NewFn, {CI->getArgOperand(0), CI->getArgOperand(1)}, Name));
-    CI->eraseFromParent();
-    return;
+    NewCall =
+        Builder.CreateCall(NewFn, {CI->getArgOperand(0), CI->getArgOperand(1)});
+    break;
 
   case Intrinsic::ctpop:
-    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, {CI->getArgOperand(0)}));
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, {CI->getArgOperand(0)});
+    break;
 
   case Intrinsic::convert_from_fp16:
-    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, {CI->getArgOperand(0)}));
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, {CI->getArgOperand(0)});
+    break;
 
   case Intrinsic::x86_xop_vfrcz_ss:
   case Intrinsic::x86_xop_vfrcz_sd:
-    CI->replaceAllUsesWith(
-        Builder.CreateCall(NewFn, {CI->getArgOperand(1)}, Name));
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, {CI->getArgOperand(1)});
+    break;
 
   case Intrinsic::x86_xop_vpermil2pd:
   case Intrinsic::x86_xop_vpermil2ps:
   case Intrinsic::x86_xop_vpermil2pd_256:
   case Intrinsic::x86_xop_vpermil2ps_256: {
     SmallVector<Value *, 4> Args(CI->arg_operands().begin(),
                                  CI->arg_operands().end());
     VectorType *FltIdxTy = cast<VectorType>(Args[2]->getType());
     VectorType *IntIdxTy = VectorType::getInteger(FltIdxTy);
     Args[2] = Builder.CreateBitCast(Args[2], IntIdxTy);
-    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, Args, Name));
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, Args);
+    break;
   }
 
   case Intrinsic::x86_sse41_ptestc:
   case Intrinsic::x86_sse41_ptestz:
   case Intrinsic::x86_sse41_ptestnzc: {
     // The arguments for these intrinsics used to be v4f32, and changed
     // to v2i64. This is purely a nop, since those are bitwise intrinsics.
     // So, the only thing required is a bitcast for both arguments.
     // First, check the arguments have the old type.
     Value *Arg0 = CI->getArgOperand(0);
     if (Arg0->getType() != VectorType::get(Type::getFloatTy(C), 4))
       return;
 
     // Old intrinsic, add bitcasts
     Value *Arg1 = CI->getArgOperand(1);
 
     Type *NewVecTy = VectorType::get(Type::getInt64Ty(C), 2);
 
     Value *BC0 = Builder.CreateBitCast(Arg0, NewVecTy, "cast");
     Value *BC1 = Builder.CreateBitCast(Arg1, NewVecTy, "cast");
 
-    CallInst *NewCall = Builder.CreateCall(NewFn, {BC0, BC1}, Name);
-    CI->replaceAllUsesWith(NewCall);
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, {BC0, BC1});
+    break;
   }
 
   case Intrinsic::x86_sse41_insertps:
   case Intrinsic::x86_sse41_dppd:
   case Intrinsic::x86_sse41_dpps:
   case Intrinsic::x86_sse41_mpsadbw:
   case Intrinsic::x86_avx_dp_ps_256:
   case Intrinsic::x86_avx2_mpsadbw: {
     // Need to truncate the last argument from i32 to i8 -- this argument models
     // an inherently 8-bit immediate operand to these x86 instructions.
     SmallVector<Value *, 4> Args(CI->arg_operands().begin(),
                                  CI->arg_operands().end());
 
     // Replace the last argument with a trunc.
     Args.back() = Builder.CreateTrunc(Args.back(), Type::getInt8Ty(C), "trunc");
-
-    CallInst *NewCall = Builder.CreateCall(NewFn, Args);
-    CI->replaceAllUsesWith(NewCall);
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, Args);
+    break;
   }
 
   case Intrinsic::thread_pointer: {
-    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, {}));
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, {});
+    break;
   }
 
   case Intrinsic::invariant_start:
   case Intrinsic::invariant_end:
   case Intrinsic::masked_load:
   case Intrinsic::masked_store: {
     SmallVector<Value *, 4> Args(CI->arg_operands().begin(),
                                  CI->arg_operands().end());
-    CI->replaceAllUsesWith(Builder.CreateCall(NewFn, Args));
-    CI->eraseFromParent();
-    return;
+    NewCall = Builder.CreateCall(NewFn, Args);
+    break;
+  }
   }
+  assert(NewCall && "Should have either set this variable or returned through "
+                    "the default case");
+  std::string Name = CI->getName();
+  if (!Name.empty()) {
+    CI->setName(Name + ".old");
+    NewCall->setName(Name);
   }
+  CI->replaceAllUsesWith(NewCall);
+  CI->eraseFromParent();
 }
 
 void llvm::UpgradeCallsToIntrinsic(Function *F) {
   assert(F && "Illegal attempt to upgrade a non-existent intrinsic.");
 
   // Check if this function should be upgraded and get the replacement function
   // if there is one.
   Function *NewFn;

llvm/trunk/test/CodeGen/Generic/overloaded-intrinsic-name.ll

 ; RUN: opt -verify -S < %s
+; RUN: opt -S < %s | FileCheck %s
 
 ; Tests the name mangling performed by the codepath following
 ; getMangledTypeStr(). Only tests that code with the various manglings
 ; run fine: doesn't actually test the mangling with the type of the
 ; arguments. Meant to serve as an example-document on how the user
 ; should do name manglings.
 
 ; Exercise the most general case, llvm_anyptr_type, using gc.relocate
entry:
       %tok = call token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1f(i64 0, i32 0, i1 ()* @return_i1, i32 0, i32 0, i32 0, i32 0, %i32* %v)
       %v-new = call %i32* @llvm.experimental.gc.relocate.p0s_i32s(token %tok,  i32 7, i32 7)
       ret %i32* %v-new
 }
 ; completely broken intrinsic naming due to needing remangling. Just use random naming to test
 
 define %i32* @test_broken_names(%i32* %v) gc "statepoint-example" {
 entry:
-      %tok = call token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.deadbeef(i64 0, i32 0, i1 ()* @return_i1, i32 0, i32 0, i32 0, i32 0, %i32* %v)
+      %tok = call fastcc token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.deadbeef(i64 0, i32 0, i1 ()* @return_i1, i32 0, i32 0, i32 0, i32 0, %i32* %v)
+; Make sure we do not destroy the calling convention when remangling
+; CHECK: fastcc
       %v-new = call %i32* @llvm.experimental.gc.relocate.beefdead(token %tok,  i32 7, i32 7)
       ret %i32* %v-new
 }
 declare zeroext i1 @return_i1()
 declare token @llvm.experimental.gc.statepoint.p0f_i1f(i64, i32, i1 ()*, i32, i32, ...)
 declare i32* @llvm.experimental.gc.relocate.p0i32(token, i32, i32)
 declare float* @llvm.experimental.gc.relocate.p0f32(token, i32, i32)
 declare [3 x i32]* @llvm.experimental.gc.relocate.p0a3i32(token, i32, i32)
 declare <3 x i32>* @llvm.experimental.gc.relocate.p0v3i32(token, i32, i32)
 declare %struct.test* @llvm.experimental.gc.relocate.p0s_struct.tests(token, i32, i32)
 declare {i64, i64, {i64}}* @llvm.experimental.gc.relocate.p0sl_i64i64sl_i64ss.test(token, i32, i32)
 declare %i32* @llvm.experimental.gc.relocate.p0s_i32s(token, i32, i32)
 declare %i32* @llvm.experimental.gc.relocate.beefdead(token, i32, i32)
 declare token @llvm.experimental.gc.statepoint.deadbeef(i64, i32, i1 ()*, i32, i32, ...)