[Clang] Emit type metadata on vtables when IRPGO instrumentation option is on. #70841

mingmingl-llvm · 2023-10-31T17:52:41Z

The motivating use case is to have type metadata on vtables if IRPGO instrumentation is on (without the requirement of -fwhole-program-vtables or -flto).

A related rfc is in https://discourse.llvm.org/t/rfc-dynamic-type-profiling-and-optimizations-in-llvm/74600

github-actions · 2023-10-31T18:05:43Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff a9b3f200159595ed8f13cc891ae34291020ba111 478e8f2191aa577660e310fc0568c03188ee8348 -- clang/lib/CodeGen/CGVTables.cpp clang/lib/CodeGen/MicrosoftCXXABI.cpp clang/test/CodeGenCXX/type-metadata.cpp

View the diff from clang-format here.

diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 32ac5809c871..dd5e117bf61e 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -4453,9 +4453,7 @@ void MicrosoftCXXABI::emitThrow(CodeGenFunction &CGF, const CXXThrowExpr *E) {
 
   // Call into the runtime to throw the exception.
   llvm::Value *Args[] = {
-    CGF.Builder.CreateBitCast(AI.getPointer(), CGM.Int8PtrTy),
-    TI
-  };
+      CGF.Builder.CreateBitCast(AI.getPointer(), CGM.Int8PtrTy), TI};
   CGF.EmitNoreturnRuntimeCallOrInvoke(getThrowFn(), Args);
 }

on. - Add test case in clang/test/Driver directory. Undo the change (added in previous commit in this PR) in clang/test/CodeGenCXX/type-metadata.cpp since the test coverage is in clang driver.

llvmbot · 2023-11-01T21:14:23Z

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Mingming Liu (minglotus-6)

Changes

The motivating use case is to have type metadata on vtables if IRPGO instrumentation is on (without the requirement of -fwhole-program-vtables or -flto).

A related rfc is in https://discourse.llvm.org/t/rfc-dynamic-type-profiling-and-optimizations-in-llvm/74600

Full diff: https://github.com/llvm/llvm-project/pull/70841.diff

3 Files Affected:

(modified) clang/lib/CodeGen/CGVTables.cpp (+1-1)
(modified) clang/lib/CodeGen/MicrosoftCXXABI.cpp (+2-1)
(added) clang/test/Driver/type-metadata.cpp (+145)

diff --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp
index 25e4b1c27932026..895e160dfa8a536 100644
--- a/clang/lib/CodeGen/CGVTables.cpp
+++ b/clang/lib/CodeGen/CGVTables.cpp
@@ -1312,7 +1312,7 @@ llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
 void CodeGenModule::EmitVTableTypeMetadata(const CXXRecordDecl *RD,
                                            llvm::GlobalVariable *VTable,
                                            const VTableLayout &VTLayout) {
-  if (!getCodeGenOpts().LTOUnit)
+  if (!getCodeGenOpts().LTOUnit && !getCodeGenOpts().hasProfileIRInstr())
     return;
 
   CharUnits ComponentWidth = GetTargetTypeStoreSize(getVTableComponentType());
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index d623f8f63ae56c4..740c0b6cb1aac04 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1673,7 +1673,8 @@ void MicrosoftCXXABI::EmitDestructorCall(CodeGenFunction &CGF,
 void MicrosoftCXXABI::emitVTableTypeMetadata(const VPtrInfo &Info,
                                              const CXXRecordDecl *RD,
                                              llvm::GlobalVariable *VTable) {
-  if (!CGM.getCodeGenOpts().LTOUnit)
+  if (!CGM.getCodeGenOpts().LTOUnit &&
+      !CGM.getCodeGenOpts().hasProfileIRInstr())
     return;
 
   // TODO: Should VirtualFunctionElimination also be supported here?
diff --git a/clang/test/Driver/type-metadata.cpp b/clang/test/Driver/type-metadata.cpp
new file mode 100644
index 000000000000000..a34b3b08e7a4493
--- /dev/null
+++ b/clang/test/Driver/type-metadata.cpp
@@ -0,0 +1,145 @@
+// Test that type metadata are emitted with -fprofile-generate
+//
+// RUN: %clang -fprofile-generate -fno-lto -target x86_64-unknown-linux -emit-llvm -S %s -o - | FileCheck %s --check-prefix=ITANIUM
+// RUN: %clang -fprofile-generate -fno-lto -target x86_64-pc-windows-msvc -emit-llvm -S %s -o - | FileCheck %s --check-prefix=MS
+
+// ITANIUM: @_ZTV1A = {{[^!]*}}, !type [[A16:![0-9]+]]
+// ITANIUM-SAME:  !type [[AF16:![0-9]+]]
+
+// ITANIUM: @_ZTV1B = {{[^!]*}}, !type  [[A32:![0-9]+]]
+// ITANIUM-SAME: !type [[AF32:![0-9]+]]
+// ITANIUM-SAME: !type [[AF40:![0-9]+]]
+// ITANIUM-SAME: !type [[AF48:![0-9]+]]
+// ITANIUM-SAME: !type [[B32:![0-9]+]]
+// ITANIUM-SAME: !type [[BF32:![0-9]+]]
+// ITANIUM-SAME: !type [[BF40:![0-9]+]]
+// ITANIUM-SAME: !type [[BF48:![0-9]+]]
+
+// ITANIUM: @_ZTV1C = {{[^!]*}}, !type [[A32]]
+// ITANIUM-SAME: !type [[AF32]]
+// ITANIUM-SAME: !type [[C32:![0-9]+]]
+// ITANIUM-SAME: !type [[CF32:![0-9]+]]
+
+// ITANIUM: @_ZTVN12_GLOBAL__N_11DE = {{[^!]*}}, !type [[A32]]
+// ITANIUM-SAME: !type [[AF32]]
+// ITANIUM-SAME: !type [[AF40]]
+// ITANIUM-SAME: !type [[AF48]]
+// ITANIUM-SAME: !type [[B32]]
+// ITANIUM-SAME: !type [[BF32]]
+// ITANIUM-SAME: !type [[BF40]]
+// ITANIUM-SAME: !type [[BF48]]
+// ITANIUM-SAME: !type [[C88:![0-9]+]]
+// ITANIUM-SAME: !type [[CF32]]
+// ITANIUM-SAME: !type [[CF40:![0-9]+]]
+// ITANIUM-SAME: !type [[CF48:![0-9]+]]
+// ITANIUM-SAME: !type [[D32:![0-9]+]]
+// ITANIUM-SAME: !type [[DF32:![0-9]+]]
+// ITANIUM-SAME: !type [[DF40:![0-9]+]]
+// ITANIUM-SAME: !type [[DF48:![0-9]+]]
+
+// ITANIUM: @_ZTCN12_GLOBAL__N_11DE0_1B = {{[^!]*}}, !type [[A32]]
+// ITANIUM-SAME: !type [[B32]]
+
+// ITANIUM: @_ZTCN12_GLOBAL__N_11DE8_1C = {{[^!]*}}, !type [[A64:![0-9]+]]
+// ITANIUM-SAME: !type [[AF64:![0-9]+]]
+// ITANIUM-SAME: !type [[C32]]
+// ITANIUM-SAME: !type [[CF64:![0-9]+]]
+
+// ITANIUM: @_ZTVZ3foovE2FA = {{[^!]*}}, !type [[A16]]
+// ITANIUM-SAME: !type [[AF16]]
+// ITANIUM-SAME: !type [[FA16:![0-9]+]]
+// ITANIUM-SAME: !type [[FAF16:![0-9]+]]
+
+// MS: comdat($"??_7A@@6B@"), !type [[A8:![0-9]+]]
+// MS: comdat($"??_7B@@6B0@@"), !type [[B8:![0-9]+]]
+// MS: comdat($"??_7B@@6BA@@@"), !type [[A8]]
+// MS: comdat($"??_7C@@6B@"), !type [[A8]]
+// MS: comdat($"??_7D@?A0x{{[^@]*}}@@6BB@@@"), !type [[B8]], !type [[D8:![0-9]+]]
+// MS: comdat($"??_7D@?A0x{{[^@]*}}@@6BA@@@"), !type [[A8]]
+// MS: comdat($"??_7FA@?1??foo@@YAXXZ@6B@"), !type [[A8]], !type [[FA8:![0-9]+]]
+
+struct A {
+  A();
+  virtual void f();
+};
+
+struct B : virtual A {
+  B();
+  virtual void g();
+  virtual void h();
+};
+
+struct C : virtual A {
+  C();
+};
+
+namespace {
+
+struct D : B, C {
+  D();
+  virtual void f();
+  virtual void h();
+};
+
+}
+
+A::A() {}
+B::B() {}
+C::C() {}
+D::D() {}
+
+void A::f() {
+}
+
+void B::g() {
+}
+
+void D::f() {
+}
+
+void D::h() {
+}
+
+void af(A *a) {
+  a->f();
+}
+
+void df1(D *d) {
+ 
+  d->f();
+}
+
+void dg1(D *d) {
+ 
+  d->g();
+}
+
+void dh1(D *d) {
+  
+  d->h();
+}
+
+void df2(D *d) {
+
+  d->f();
+}
+
+void df3(D *d) {
+
+  d->f();
+}
+
+D d;
+
+void foo() {
+  df1(&d);
+  dg1(&d);
+  dh1(&d);
+  df2(&d);
+  df3(&d);
+
+  struct FA : A {
+    void f() {}
+  } fa;
+  af(&fa);
+}

mingmingl-llvm · 2023-11-01T21:17:16Z

clang/test/Driver/type-metadata.cpp

+// Test that type metadata are emitted with -fprofile-generate
+//
+// RUN: %clang -fprofile-generate -fno-lto -target x86_64-unknown-linux -emit-llvm -S %s -o - | FileCheck %s --check-prefix=ITANIUM
+// RUN: %clang -fprofile-generate -fno-lto -target x86_64-pc-windows-msvc -emit-llvm -S %s -o - | FileCheck %s --check-prefix=MS


This test is forked from https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenCXX/type-metadata.cpp.

It's currently put under directory clang/test/Driver since -fprofile-generate is a driver option.

I'm wondering if it's more idiomatic to create the corresponding clang compiler options for -fprofile-generate for what I'm trying to do here? That way I could update clang/test/CodeGenCXX/type-metadata.cpp for test coverage.

Yes I think that would be more typical and better imo (i.e. use %clang_cc to test)

thanks! I updated the existing test (and removed the forked one) by only generating !type annotations iff clang cc1 option -fprofile-instrument is set to llvm (the FDO build). CS-FDO and clang instrumentation sets this cc1 option differently, and it's intentional for this PR not to emit !type in these two settings mainly because there is no use case.

A longer story from some interesting findings when I tried to figured out how clang options work. Currently, clang driver accepts two options for FDO

-fprofile-generate (a Flag option that doesn't accept value according to https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option

-fprofile-generate= (a Joined option that accepts a value)

I initially add CC1Option to both (and added corresponding codegen options); and during this process found -fprofile-instrument=llvm is true if clang user either specified -fprofile-generate or -fprofile-generate=path/to (but not CS-FDO or clang instrumentation FDO), so decided to just rely on this.

…rking it to clang/test/Driver

teresajohnson · 2023-11-03T18:17:44Z

clang/test/CodeGenCXX/type-metadata.cpp

-// RUN: %clang_cc1 -fexperimental-relative-c++-abi-vtables -flto -flto-unit -triple x86_64-unknown-linux -fvisibility=hidden -fsanitize=cfi-vcall -fsanitize-trap=cfi-vcall -fwhole-program-vtables -emit-llvm -o - %s | FileCheck --check-prefix=CFI --check-prefix=CFI-VT --check-prefix=ITANIUM --check-prefix=RV-MD --check-prefix=TC-ITANIUM %s
+// RUN: %clang_cc1 -fexperimental-relative-c++-abi-vtables -flto -flto-unit -triple x86_64-unknown-linux -fvisibility=hidden -fsanitize=cfi-vcall -fsanitize-trap=cfi-vcall -fwhole-program-vtables -emit-llvm -o - %s | FileCheck --check-prefix=CFI --check-prefix=CFI-VT --check-prefix=ITANIUM --check-prefix=ITANIUM-TYPEMETADATA --check-prefix=RV-MD --check-prefix=TC-ITANIUM %s
+
+// Tests type metadata are annotated on vtables with `-profile-instrument=llvm` (which is equivalent as clang driver option `-fprofile-generate` without `-fcs-profile-generate`):


nit: "equivalent to"

teresajohnson · 2023-11-03T18:20:37Z

clang/test/CodeGenCXX/type-metadata.cpp


+// Test !type doesn't exist in the generated IR.
+// NOTYPEMD-NOT: !type


Instead of this do an --implicit-check-not='!type' on the FileCheck invocations

teresajohnson · 2023-11-03T18:25:22Z

clang/test/CodeGenCXX/type-metadata.cpp


 // Tests for the whole-program-vtables feature:
-// RUN: %clang_cc1 -fexperimental-relative-c++-abi-vtables -flto -flto-unit -triple x86_64-unknown-linux -fvisibility=hidden -fwhole-program-vtables -emit-llvm -o - %s | FileCheck --check-prefix=VTABLE-OPT --check-prefix=ITANIUM -check-prefix=RV-MD --check-prefix=TT-ITANIUM-HIDDEN %s
+// RUN: %clang_cc1 -fexperimental-relative-c++-abi-vtables -flto -flto-unit -triple x86_64-unknown-linux -fvisibility=hidden -fwhole-program-vtables -emit-llvm -o - %s | FileCheck --check-prefix=VTABLE-OPT --check-prefix=ITANIUM --check-prefix=ITANIUM-TYPEMETADATA -check-prefix=RV-MD --check-prefix=TT-ITANIUM-HIDDEN %s


It isn't clear to me why a new ITANIUM-TYPEMETADATA check prefix needed to be added. There is now afaict only one check using the old "ITANIUM" check flag and it is just a vtable definition - is the definition of that vtable different with -profile-instrument=llvm? If it is, maybe combine that with ITANIUM-DEFAULTVIS which also only has one check and identical to the remaining ITANIUM check. Otherwise can the new ITANIUM-TYPEMETADATA be removed and just use ITANIUM?

The previously left vtable should have !type. I fixed it in the updated commit.

ITANIUM prefix was used to check functions are hidden besides !type on vtable definitions previously. I used 'Ctrl + F' in browser to search for "fvisibility=hidden" and "--check-prefix=ITANIUM ", both have 10 counts and appear on the same line.

In the updated test

Add a prefix ITANIUM-COMMON-MD. This prefix is used to check !type is annotated on vtable definitions, and used by both non-relative and relative vtable 'RUN lines'

Add a prefix 'ITANIUM-HIDDEN'. This prefix is to check contents for '-fvisibility=hidden'.

Replace existing 'ITANIUM-MD' with 'ITANIUM-NO-RV-MD', indicating the lines are unique to non-relative vtable scenarios.

This way,

the existing 'RUN' lines that used to have --check-prefix=ITANIUM' now have '--check-prefix=ITANIUM-HIDDEN --check-prefix=ITANIUM-COMMON-MD

the new 'RUN' lines (for IR instrumentation) could use '--check-prefix=ITANIUM-COMMON-MD' for itanium vtable check.

teresajohnson · 2023-11-03T18:25:49Z

clang/lib/CodeGen/CGVTables.cpp

@@ -1312,7 +1312,7 @@ llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
 void CodeGenModule::EmitVTableTypeMetadata(const CXXRecordDecl *RD,
                                           llvm::GlobalVariable *VTable,
                                           const VTableLayout &VTLayout) {
-  if (!getCodeGenOpts().LTOUnit)
+  if (!getCodeGenOpts().LTOUnit && !getCodeGenOpts().hasProfileIRInstr())


Add a comment here and in the other file noting why it is added for IR instrumentation.

teresajohnson

Somehow I missed those other uses of the ITANIUM prefix. Thanks for cleaning it up though I think the new names are more descriptive.

mingmingl-llvm · 2023-11-03T22:56:38Z

Somehow I missed those other uses of the ITANIUM prefix. Thanks for cleaning it up though I think the new names are more descriptive.

yep! I hope the new names helps more or less going forward. Thanks for the feedbacks!

[Clang]Emit type metadata when -fprofile-generate is on

99abebc

mingmingl-llvm changed the title ~~[Clang]Emit type metadata when -fprofile-generate is on~~ [Clang]Emit type metadata when IRPGO is used Oct 31, 2023

Emit type metadata on vtables if IRPGO instrumentation or profile use is

5b53f31

on. - Add test case in clang/test/Driver directory. Undo the change (added in previous commit in this PR) in clang/test/CodeGenCXX/type-metadata.cpp since the test coverage is in clang driver.

mingmingl-llvm changed the title ~~[Clang]Emit type metadata when IRPGO is used~~ [Clang] Emit type metadata on vtables when IRPGO option is on. Nov 1, 2023

mingmingl-llvm mentioned this pull request Nov 1, 2023

[InstrFDO][TypeProf] Implement binary instrumentation and profile read/write #66825

Merged

undo the change for -fprofile-use

33927fa

mingmingl-llvm changed the title ~~[Clang] Emit type metadata on vtables when IRPGO option is on.~~ [Clang] Emit type metadata on vtables when IRPGO instrumentation option is on. Nov 1, 2023

mingmingl-llvm requested review from pcc and teresajohnson November 1, 2023 21:13

mingmingl-llvm marked this pull request as ready for review November 1, 2023 21:13

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:codegen IR generation bugs: mangling, exceptions, etc. labels Nov 1, 2023

mingmingl-llvm commented Nov 1, 2023

View reviewed changes

mingmingl-llvm added 2 commits November 2, 2023 21:55

modify clang/test/CodeGenCXX/type-metadata.cpp directly instead of fo…

87b8de5

…rking it to clang/test/Driver

a real rm of forked test in clang/test/Driver dir

3fcf25d

teresajohnson reviewed Nov 3, 2023

View reviewed changes

resolve comments

478e8f2

teresajohnson approved these changes Nov 3, 2023

View reviewed changes

mingmingl-llvm merged commit 34c0d32 into llvm:main Nov 3, 2023

mingmingl-llvm deleted the clang branch November 3, 2023 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Emit type metadata on vtables when IRPGO instrumentation option is on. #70841

[Clang] Emit type metadata on vtables when IRPGO instrumentation option is on. #70841

Uh oh!

mingmingl-llvm commented Oct 31, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Oct 31, 2023 •

edited

Loading

Uh oh!

llvmbot commented Nov 1, 2023 •

edited

Loading

Uh oh!

mingmingl-llvm Nov 1, 2023

Uh oh!

teresajohnson Nov 1, 2023

Uh oh!

mingmingl-llvm Nov 3, 2023 •

edited

Loading

Uh oh!

teresajohnson Nov 3, 2023

Uh oh!

mingmingl-llvm Nov 3, 2023

Uh oh!

teresajohnson Nov 3, 2023

Uh oh!

mingmingl-llvm Nov 3, 2023

Uh oh!

teresajohnson Nov 3, 2023

Uh oh!

mingmingl-llvm Nov 3, 2023 •

edited

Loading

Uh oh!

teresajohnson Nov 3, 2023

Uh oh!

mingmingl-llvm Nov 3, 2023

Uh oh!

teresajohnson left a comment

Uh oh!

mingmingl-llvm commented Nov 3, 2023

Uh oh!

Uh oh!


		// Test !type doesn't exist in the generated IR.
		// NOTYPEMD-NOT: !type

[Clang] Emit type metadata on vtables when IRPGO instrumentation option is on. #70841

[Clang] Emit type metadata on vtables when IRPGO instrumentation option is on. #70841

Uh oh!

Conversation

mingmingl-llvm commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm commented Nov 3, 2023

Uh oh!

Uh oh!

mingmingl-llvm commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

llvmbot commented Nov 1, 2023 •

edited

Loading

mingmingl-llvm Nov 3, 2023 •

edited

Loading

mingmingl-llvm Nov 3, 2023 •

edited

Loading