[IR] Have Value::takeName() transfer intrinsic properties

[frasercrmck]

Summary

Functions are considered intrinsics if their names begin with "llvm.",
even if they don't have a valid intrinsic ID. This is a cached property,
stored in GlobalValue's HasLLVMReservedName.

When calling Value::setName(), this property is recalculated. However,
this property is not recalculated when taking names from another Value.
While it's not documented as doing such, the semantics of "takeName"
suggest that A.takeName(B) is the same as A.setName(B.getName())
followed by B.setName("").

This patch updates Value::takeName to act in this fashion, to avoid any
unexpected behaviour: The new function is an intrinsic since it now starts with
"llvm." and the old function is no longer considered an intrinsic, since it no
longer begins with "llvm.".

[aeubanks]

what's the use case for having a function take the name of an intrinsic function?

[frasercrmck]

In D128913#3623784, @aeubanks wrote:

what's the use case for having a function take the name of an intrinsic function?

It's a good question. I can't think of a reason you'd do this for intrinsics with intrinsic IDs, but if the meaning of intrinsic is indeed "starts with 'llvm.'", as is documented, then I think we should transfer that property over when naming functions in all cases. It's unintuitive to me to have a function beginning with "llvm." be an intrinsic if it's named through one method but not another.

As a more concrete example, downstream we have some passes which add parameters to functions, remove parameters from functions, change parameter types, etc., so we create new functions from old ones and this means we generally take the name. If, for whatever reason, we had a user using a "custom" intrinsic (if I may call them that - just a function starting with "llvm.") so they can make use of intrinsic semantics (e.g., they can use intrinsic-only function parameters like immarg or elementtype) then if we drop the fact that it's an intrinsic then at the very least the verifier would complain. That's how I originally caught this issue.

[aeubanks]

was worried about compile time issues, seems fine
https://llvm-compile-time-tracker.com/compare.php?from=a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb&to=3262dfdab15c7017935961c06b127ec9ac8dc7fd&stat=instructions

I'd sort of prefer going the opposite route of https://reviews.llvm.org/D129015 and banning setName/takeName on intrinsics, that seems less error-prone
it seems weird to create a new intrinsic declaration that has the same name as a previous intrinsic

llvm/lib/IR/Value.cpp

void Value::takeName(Value *V) {
 
   // If these values are both in the same symtab, we can do this very fast.
   // This works even if both values have no symtab yet.
   if (ST == VST) {
     // Take the name!
     setValueName(V->getValueName());
     V->setValueName(nullptr);
     getValueName()->setValue(this);
+    if (Function *F = dyn_cast<Function>(V))
+      F->recalculateIntrinsicID();
+    if (Function *F = dyn_cast<Function>(this))
+      F->recalculateIntrinsicID();
     return;
   }
 
   // Otherwise, things are slightly more complex.  Remove V's name from VST and
   // then reinsert it into ST.
 
   if (VST)
     VST->removeValueName(V->getValueName());
   setValueName(V->getValueName());
   V->setValueName(nullptr);
   getValueName()->setValue(this);
 
+  if (Function *F = dyn_cast<Function>(V))
+    F->recalculateIntrinsicID();
+  if (Function *F = dyn_cast<Function>(this))
+    F->recalculateIntrinsicID();
+
   if (ST)
     ST->reinsertValue(this);
 }
 
 #ifndef NDEBUG
 std::string Value::getNameOrAsOperand() const {
   if (!getName().empty())
     return std::string(getName());

llvm/unittests/IR/ValueTest.cpp

TEST(ValueTest, replaceUsesOutsideBlock) {
   A->replaceUsesOutsideBlock(B, Entry);
   // These users are in Entry so shouldn't be changed.
   ASSERT_TRUE(C->getOperand(0) == cast<Value>(A));
   ASSERT_TRUE(EntryDbg->getValue(0) == cast<Value>(A));
   // These users are outside Entry so should be changed.
   ASSERT_TRUE(ExitDbg->getValue(0) == cast<Value>(B));
   ASSERT_TRUE(Ret->getOperand(0) == cast<Value>(B));
 }
+
+TEST(ValueTest, IntrinsicFunctionNames) {
+  LLVMContext C;
+  const char *ModuleString = "declare void @llvm.foo(i32)\n";
+
+  SMDiagnostic Err;
+  std::unique_ptr<Module> M = parseAssemblyString(ModuleString, Err, C);
+
+  // Any function beginning with "llvm." has an LLVM-reserved name and is
+  // considered an intrinsic.
+  Function *F = M->getFunction("llvm.foo");
+  EXPECT_TRUE(F->isIntrinsic());
+
+  // Create an intrinsic function explicitly. Check that it is indeed an
+  // intrinsic.
+  auto *FTy = F->getFunctionType();
+  auto *F1 = Function::Create(FTy, GlobalValue::ExternalLinkage, "llvm.foo", *M);
+  EXPECT_TRUE(F1->isIntrinsic());
+
+  // Create a new non-intrinsic function. Set the name from a known intrinsic -
+  // this should become an intrinsic. The old function should still be an
+  // intrinsic.
+  auto *F2 = Function::Create(FTy, GlobalValue::ExternalLinkage, "bar", *M);
+  F2->setName(F1->getName());
+  EXPECT_TRUE(F2->isIntrinsic());
+  EXPECT_TRUE(F1->isIntrinsic());
+
+  // Create a new non-intrinsic function. *Take* the name from an intrinsic -
+  // this should become be an intrinsic. The old function should no longer be
+  // an intrinsic.
+  auto *F3 = Function::Create(FTy, GlobalValue::ExternalLinkage, "baz", *M);
+  F3->takeName(F);
+  EXPECT_FALSE(F->isIntrinsic());
+  EXPECT_TRUE(F3->isIntrinsic());
+
+  F->setName("baz");
+  EXPECT_FALSE(F->isIntrinsic());
+
+  // Have an intrinsic take the name of a non-intrinsic and neither are
+  // intrinsics afterwards.
+  F3->takeName(F);
+  EXPECT_FALSE(F3->isIntrinsic());
+  EXPECT_FALSE(F->isIntrinsic());
+}
 } // end anonymous namespace