[CGProfile] allows undef in metadata node storing function pointers

[ychen]

Summary

Commits like e5d958c may RAUW with undef value. It seems better to allow
this pattern in IR verifier rather than fixing the RAUWs for e5d958c and
future commits.

[tejohnson]

Since it looks like there are a fair number of places in LLVM that replace metadata uses with undef, is it the case that the verifier handles these more gracefully elsewhere?

[dexonsmith]

It used to be that when GlobalValue-derived Constants were removed, then metadata would point at null instead. Not sure if that changed at some point, or if this is a unique code path, but it's not clear whether having two possible "no value" values (undef vs. null) is a good idea.

Can you explain why https://reviews.llvm.org/rGe5d958c45629ccd2f5b5f7432756be1d0fcf052c used undef instead of null? Should it use null instead?

[dexonsmith]

In D108642#2962880, @dexonsmith wrote:

It used to be that when GlobalValue-derived Constants were removed, then metadata would point at null instead. Not sure if that changed at some point, or if this is a unique code path, but it's not clear whether having two possible "no value" values (undef vs. null) is a good idea.

Can you explain why https://reviews.llvm.org/rGe5d958c45629ccd2f5b5f7432756be1d0fcf052c used undef instead of null? Should it use null instead?

Ah, that was the commit that changed it. It used to hit this logic:

Value::~Value() {
  // Notify all ValueHandles (if present) that this value is going away.
  if (HasValueHandle)
    ValueHandleBase::ValueIsDeleted(this);
  if (isUsedByMetadata())
    ValueAsMetadata::handleDeletion(this);

  // Remove associated metadata from context.
  if (HasMetadata)
    clearMetadata();

Maybe that commit should be reverted.

[ychen]

In D108642#2962903, @dexonsmith wrote:

In D108642#2962880, @dexonsmith wrote:

It used to be that when GlobalValue-derived Constants were removed, then metadata would point at null instead. Not sure if that changed at some point, or if this is a unique code path, but it's not clear whether having two possible "no value" values (undef vs. null) is a good idea.

Can you explain why https://reviews.llvm.org/rGe5d958c45629ccd2f5b5f7432756be1d0fcf052c used undef instead of null? Should it use null instead?

Ah, that was the commit that changed it. It used to hit this logic:

Value::~Value() {
  // Notify all ValueHandles (if present) that this value is going away.
  if (HasValueHandle)
    ValueHandleBase::ValueIsDeleted(this);
  if (isUsedByMetadata())
    ValueAsMetadata::handleDeletion(this);

  // Remove associated metadata from context.
  if (HasMetadata)
    clearMetadata();

Maybe that commit should be reverted.

Thank you! It looks like so indeed. Basically ReplaceableMetadataImpl::replaceAllUsesWith removed the null Value from the metadata nodes, right?

llvm/lib/IR/Verifier.cpp

Verifier::visitModuleFlag(const MDNode *Op,
   }
 }
 
 void Verifier::visitModuleFlagCGProfileEntry(const MDOperand &MDO) {
   auto CheckFunction = [&](const MDOperand &FuncMDO) {
     if (!FuncMDO)
       return;
     auto F = dyn_cast<ValueAsMetadata>(FuncMDO);
-    Assert(F && isa<Function>(F->getValue()->stripPointerCasts()),
+    Assert(F, "expected a Value", FuncMDO);
+    auto VF = F->getValue()->stripPointerCasts();
+    Assert(isa<Function>(VF) || isa<UndefValue>(VF),
            "expected a Function or null", FuncMDO);
   };
   auto Node = dyn_cast_or_null<MDNode>(MDO);
   Assert(Node && Node->getNumOperands() == 3, "expected a MDNode triple", MDO);
   CheckFunction(Node->getOperand(0));
   CheckFunction(Node->getOperand(1));
   auto Count = dyn_cast_or_null<ConstantAsMetadata>(Node->getOperand(2));
   Assert(Count && Count->getType()->isIntegerTy(),

llvm/lib/Target/TargetLoweringObjectFile.cpp

void TargetLoweringObjectFile::emitCGProfileMetadata(MCStreamer &Streamer,
 
   if (!CFGProfile)
     return;
 
   auto GetSym = [this](const MDOperand &MDO) -> MCSymbol * {
     if (!MDO)
       return nullptr;
     auto *V = cast<ValueAsMetadata>(MDO);
-    const Function *F = cast<Function>(V->getValue()->stripPointerCasts());
-    if (F->hasDLLImportStorageClass())
+    const Function *F = dyn_cast<Function>(V->getValue()->stripPointerCasts());
+    if (!F || F->hasDLLImportStorageClass())
       return nullptr;
     return TM->getSymbol(F);
   };
 
   for (const auto &Edge : CFGProfile->operands()) {
     MDNode *E = cast<MDNode>(Edge);
     const MCSymbol *From = GetSym(E->getOperand(0));
     const MCSymbol *To = GetSym(E->getOperand(1));

llvm/test/Verifier/module-flags-cgprofile.ll

 ; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
 
 declare void @b()
 declare void @a()
 
 !llvm.module.flags = !{!0}
 
 !0 = !{i32 5, !"CG Profile", !1}
-!1 = !{!2, !"", !3, !4, !5, !6, !7, !8}
+!1 = !{!2, !"", !3, !4, !5, !6, !7, !8, !9}
 !2 = !{void ()* @a, void ()* @b, i64 32}
 !3 = !{void ()* @a, void ()* @b}
 !4 = !{void ()* @a, void ()* @b, i64 32, i64 32}
 !5 = !{!"a", void ()* @b, i64 32}
 !6 = !{void ()* @a, !"b", i64 32}
 !7 = !{void ()* @a, void ()* @b, !""}
 !8 = !{void ()* @a, void ()* @b, null}
+!9 = !{void ()* @a, void ()* undef, i64 32}
 
 ; CHECK: expected a MDNode triple
 ; CHECK: !""
 ; CHECK: expected a MDNode triple
 ; CHECK: !3 = !{void ()* @a, void ()* @b}
 ; CHECK: expected a MDNode triple
 ; CHECK: !4 = !{void ()* @a, void ()* @b, i64 32, i64 32}
-; CHECK: expected a Function or null
+; CHECK: expected a Value
 ; CHECK: !"a"
-; CHECK: expected a Function or null
+; CHECK: expected a Value
 ; CHECK: !"b"
 ; CHECK: expected an integer constant
 ; CHECK: !""
 ; CHECK: expected an integer constant