[CGProfile] allows bitcast in metadata node storing function pointers

[ychen]

Summary

For example, during RAUW in IRMover, the Function ValueAsMetadata in "CG Profile" could become bitcast.

[ychen]

ping

[ychen]

ping. This is relatively minor, but it still breaks some of our use cases. Does this look good?

[tejohnson]

It seems a bit odd to have/need a custom fixup for one type of MDNode here. Can visitModuleFlagCGProfileEntry be fixed to handle the bitcast?

[ychen]

In D88433#2347480, @tejohnson wrote:

It seems a bit odd to have/need a custom fixup for one type of MDNode here. Can visitModuleFlagCGProfileEntry be fixed to handle the bitcast?

It is indeed a bit odd but that's close to where the artifact is produced. Debug metadata nodes involving globals/functions could potentially have this issue too (either their verification doesn't enforce similar logic as CGProfile node does or they just don't care I'm not sure).

Fixing visitModuleFlagCGProfileEntry works too although it addresses the consumer rather than the producer, which logically also seems odd. If this is the preferred method to address the issue, I have no strong opinion against it.

[tejohnson]

In D88433#2347753, @ychen wrote:

In D88433#2347480, @tejohnson wrote:

It seems a bit odd to have/need a custom fixup for one type of MDNode here. Can visitModuleFlagCGProfileEntry be fixed to handle the bitcast?

It is indeed a bit odd but that's close to where the artifact is produced. Debug metadata nodes involving globals/functions could potentially have this issue too (either their verification doesn't enforce similar logic as CGProfile node does or they just don't care I'm not sure).

Fixing visitModuleFlagCGProfileEntry works too although it addresses the consumer rather than the producer, which logically also seems odd. If this is the preferred method to address the issue, I have no strong opinion against it.

Barring other issues from the way the metadata nodes were constructed, I guess I would prefer the change to the Verifier. It seems a bit odd to be fixing up this one metadata type here. It would be interesting to know how or why this is not an issue for the debug metadata nodes, but the fix doesn't need to wait on that.

[ychen]

• revert fixup in IRMover
• allow CGProfile metadata node representing edge to use bitcast.

[tejohnson]

LGTM with a couple minor changes noted below, also the description needs an update as it still references fixing up the nodes.

llvm/test/Transforms/FunctionImport/cg_profile.ll

3

Comment is stale.

7

I guess the test just makes sure this doesn't fail. Would be good to do a check on the output file via llvm-dis that it in fact does have the CG Profile metadata node and that it looks as expected (which I guess now will have the bitcast).

[ychen]

• address comments: update comment, check output making sure bitcast is there.

[ychen]

• formatting

[tejohnson]

LGTM again except for the patch title and summary which need an update. Go ahead and commit once those are updated.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

void TargetLoweringObjectFileELF::emitModuleMetadata(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());
+    const Function *F = cast<Function>(V->getValue()->stripPointerCasts());
     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));
     // Skip null functions. This can happen if functions are dead stripped after

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()), "expected a Function or null",
-           FuncMDO);
+    Assert(F && isa<Function>(F->getValue()->stripPointerCasts()),
+           "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(),
          "expected an integer constant", Node->getOperand(2));

llvm/test/Transforms/FunctionImport/Inputs/cg_profile.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%class.A = type { [16 x i8] }
+
+define void @bar(%class.A*) {
+  ret void
+}
+
+!llvm.module.flags = !{!1}
+
+!1 = !{i32 1, !"EnableSplitLTOUnit", i32 0}

llvm/test/Transforms/FunctionImport/cg_profile.ll

+; Check that bitcast in "CG Profile" related metadata nodes (in this test case,
+; generated during function importing in IRMover's RAUW operations) are accepted
+; by verifier.
+; RUN: opt -cg-profile -module-summary %s -o %t.bc
+; RUN: opt -module-summary %p/Inputs/cg_profile.ll -o %t2.bc
+; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
+; RUN: opt -function-import -print-imports -summary-file %t3.thinlto.bc %t.bc \
+; RUN:   -S 2>&1 | FileCheck %s
+
+; CHECK:      !0 = !{i32 1, !"EnableSplitLTOUnit", i32 0}
+; CHECK-NEXT: !1 = !{i32 5, !"CG Profile", !2}
+; CHECK-NEXT: !2 = !{!3}
+; CHECK-NEXT: !3 = !{void ()* @foo, void (%class.A*)* bitcast (void (%class.A.0*)* @bar to void (%class.A*)*), i64 2753}
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; %class.A is defined differently in %p/Inputs/cg_profile.ll. This is to trigger
+; bitcast.
+%class.A = type { i8 }
+
+define void @foo() !prof !2 {
+  call void @bar(%class.A* null)
+  ret void
+}
+
+declare void @bar(%class.A*)
+
+!llvm.module.flags = !{!1}
+
+!1 = !{i32 1, !"EnableSplitLTOUnit", i32 0}
+!2 = !{!"function_entry_count", i64 2753}