[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 181587.
xur added a comment.

Update the patch to sync with  https://reviews.llvm.org/D54175


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54176/new/

https://reviews.llvm.org/D54176

Files:
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotestir.proftext
  test/CodeGen/Inputs/pgotestir_cs.proftext
  test/CodeGen/cspgo-instrumentation.c
  test/CodeGen/cspgo-instrumentation_thinlto.c

Index: test/CodeGen/cspgo-instrumentation_thinlto.c
===
--- test/CodeGen/cspgo-instrumentation_thinlto.c
+++ test/CodeGen/cspgo-instrumentation_thinlto.c
@@ -0,0 +1,52 @@
+// Test if CSPGO instrumentation and use pass are invoked in thinlto.
+//
+// RUN: rm -rf %t && mkdir %t
+// RUN: llvm-profdata merge -o %t/noncs.profdata %S/Inputs/pgotestir.proftext
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked in PreLink.
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/noncs.profdata -fprofile-instrument=csllvm %s -fprofile-instrument-path=default.profraw -flto=thin -mllvm -debug-pass=Structure -emit-llvm-bc -o %t/foo_fe.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/noncs.profdata -fprofile-instrument=csllvm %s -fprofile-instrument-path=default.profraw  -flto=thin -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm-bc -o %t/foo_fe_pm.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE: PGOInstrumentationUsePass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE: PGOInstrumentationGenCreateVarPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NOT: PGOInstrumentationGenPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM: Running pass: PGOInstrumentationGenCreateVar
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM-NOT: Running pass: PGOInstrumentationGen on
+//
+// RUN: llvm-lto -thinlto -o %t/foo %t/foo_fe.bc
+// RUN: llvm-lto -thinlto -o %t/foo_pm %t/foo_fe_pm.bc
+// Ensure Pass PGOInstrumentationGenPass is invoked in PostLink.
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe.bc -fthinlto-index=%t/foo.thinlto.bc -fprofile-instrument=csllvm -fprofile-instrument-path=default.profraw  -flto=thin -emit-llvm -mllvm -debug-pass=Structure -o - |& FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe_pm.bc -fthinlto-index=%t/foo_pm.thinlto.bc -fexperimental-new-pass-manager -fdebug-pass-manager  -fprofile-instrument=csllvm -fprofile-instrument-path=default.profraw  -flto=thin -emit-llvm -o - |& FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NOT: PGOInstrumentationUsePass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NOT: PGOInstrumentationGenCreateVarPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST: PGOInstrumentationGenPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM-NOT: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM-NOT: Running pass: PGOInstrumentationGenCreateVar
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM: Running pass: PGOInstrumentationGen on
+//
+// RUN: llvm-profdata merge -o %t/cs.profdata %S/Inputs/pgotestir_cs.proftext
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked Once in PreLink.
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/cs.profdata %s -flto=thin -mllvm -debug-pass=Structure -emit-llvm-bc -o %t/foo_fe.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/cs.profdata %s -flto=thin -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm-bc -o %t/foo_fe_pm.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE: PGOInstrumentationUsePass
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NOT: PGOInstrumentationUsePass
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM-NOT: Running pass: PGOInstrumentationUse
+//
+// RUN: llvm-lto -thinlto -o %t/foo %t/foo_fe.bc
+// RUN: llvm-lto -thinlto -o %t/foo_pm %t/foo_fe_pm.bc
+// Ensure Pass PGOInstrumentationUSEPass is invoked in PostLink.
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe.bc -fthinlto-index=%t/foo.thinlto.bc -fprofile-instrument-use-path=%t_cs.profdata -flto=thin -emit-llvm -mllvm -debug-pass=Structure -o - 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-POST
+// RUN: %clang_cc1 -O2 -x ir %t/

[PATCH] D58737: [InstrProf] Use separate comdat group for data and counters

[Rong Xu via Phabricator via cfe-commits]

xur added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:756
+  // For COFF, the comdat group name must be the name of a symbol in the
+  // group. Use the counter variable name.
+  Cmdt = M->getOrInsertComdat(

So with the patch, COFF and ELF have the same way of naming. Can we simplify 
the code by hosting the common code?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58737/new/

https://reviews.llvm.org/D58737



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58737: [InstrProf] Use separate comdat group for data and counters

[Rong Xu via Phabricator via cfe-commits]

xur accepted this revision.
xur added a comment.
This revision is now accepted and ready to land.

lgtm.

BTW, I'm in the process of committing D54175 . 
After that patch, PGO instrumentation can be called after the main inlining. I 
don't think it will conflict anything in this patch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58737/new/

https://reviews.llvm.org/D58737



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 188955.
xur added a comment.

Integrated Teresa's suggestion to change the err() to assert.
Added documentation change suggested by David.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54176/new/

https://reviews.llvm.org/D54176

Files:
  docs/UsersManual.rst
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotestir.proftext
  test/CodeGen/Inputs/pgotestir_cs.proftext
  test/CodeGen/cspgo-instrumentation.c
  test/CodeGen/cspgo-instrumentation_lto.c
  test/CodeGen/cspgo-instrumentation_thinlto.c

Index: test/CodeGen/cspgo-instrumentation_thinlto.c
===
--- test/CodeGen/cspgo-instrumentation_thinlto.c
+++ test/CodeGen/cspgo-instrumentation_thinlto.c
@@ -0,0 +1,52 @@
+// Test if CSPGO instrumentation and use pass are invoked in thinlto.
+//
+// RUN: rm -rf %t && mkdir %t
+// RUN: llvm-profdata merge -o %t/noncs.profdata %S/Inputs/pgotestir.proftext
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked in PreLink.
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/noncs.profdata -fprofile-instrument=csllvm %s -fprofile-instrument-path=default.profraw -flto=thin -mllvm -debug-pass=Structure -emit-llvm-bc -o %t/foo_fe.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/noncs.profdata -fprofile-instrument=csllvm %s -fprofile-instrument-path=default.profraw  -flto=thin -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm-bc -o %t/foo_fe_pm.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE: PGOInstrumentationUsePass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE: PGOInstrumentationGenCreateVarPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NOT: PGOInstrumentationGenPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM: Running pass: PGOInstrumentationGenCreateVar
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM-NOT: Running pass: PGOInstrumentationGen on
+//
+// RUN: llvm-lto -thinlto -o %t/foo %t/foo_fe.bc
+// RUN: llvm-lto -thinlto -o %t/foo_pm %t/foo_fe_pm.bc
+// Ensure Pass PGOInstrumentationGenPass is invoked in PostLink.
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe.bc -fthinlto-index=%t/foo.thinlto.bc -fprofile-instrument=csllvm -fprofile-instrument-path=default.profraw  -flto=thin -emit-llvm -mllvm -debug-pass=Structure -o - 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe_pm.bc -fthinlto-index=%t/foo_pm.thinlto.bc -fexperimental-new-pass-manager -fdebug-pass-manager  -fprofile-instrument=csllvm -fprofile-instrument-path=default.profraw  -flto=thin -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NOT: PGOInstrumentationUsePass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NOT: PGOInstrumentationGenCreateVarPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST: PGOInstrumentationGenPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM-NOT: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM-NOT: Running pass: PGOInstrumentationGenCreateVar
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM: Running pass: PGOInstrumentationGen on
+//
+// RUN: llvm-profdata merge -o %t/cs.profdata %S/Inputs/pgotestir_cs.proftext
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked Once in PreLink.
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/cs.profdata %s -flto=thin -mllvm -debug-pass=Structure -emit-llvm-bc -o %t/foo_fe.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/cs.profdata %s -flto=thin -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm-bc -o %t/foo_fe_pm.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE: PGOInstrumentationUsePass
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NOT: PGOInstrumentationUsePass
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM-NOT: Running pass: PGOInstrumentationUse
+//
+// RUN: llvm-lto -thinlto -o %t/foo %t/foo_fe.bc
+// RUN: llvm-lto -thinlto -o %t/foo_pm %t/foo_fe_pm.bc
+// Ensure Pass PGOInstrumentationUSEPass is invoked in PostLink.
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe.bc -fthinlto-index=%t/foo.thinlto.bc -fprofile-instrument-use-path=%t/cs.profdata -flto=thin -emit-llvm -mllvm -debug-pass=Structure -o - 2>&1 | FileCheck %s -

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 189167.
xur added a comment.

Added usage example to UserManual.rst suggested by David.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54176/new/

https://reviews.llvm.org/D54176

Files:
  docs/UsersManual.rst
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotestir.proftext
  test/CodeGen/Inputs/pgotestir_cs.proftext
  test/CodeGen/cspgo-instrumentation.c
  test/CodeGen/cspgo-instrumentation_lto.c
  test/CodeGen/cspgo-instrumentation_thinlto.c

Index: test/CodeGen/cspgo-instrumentation_thinlto.c
===
--- test/CodeGen/cspgo-instrumentation_thinlto.c
+++ test/CodeGen/cspgo-instrumentation_thinlto.c
@@ -0,0 +1,52 @@
+// Test if CSPGO instrumentation and use pass are invoked in thinlto.
+//
+// RUN: rm -rf %t && mkdir %t
+// RUN: llvm-profdata merge -o %t/noncs.profdata %S/Inputs/pgotestir.proftext
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked in PreLink.
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/noncs.profdata -fprofile-instrument=csllvm %s -fprofile-instrument-path=default.profraw -flto=thin -mllvm -debug-pass=Structure -emit-llvm-bc -o %t/foo_fe.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/noncs.profdata -fprofile-instrument=csllvm %s -fprofile-instrument-path=default.profraw  -flto=thin -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm-bc -o %t/foo_fe_pm.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE: PGOInstrumentationUsePass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE: PGOInstrumentationGenCreateVarPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NOT: PGOInstrumentationGenPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM: Running pass: PGOInstrumentationGenCreateVar
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM-NOT: Running pass: PGOInstrumentationGen on
+//
+// RUN: llvm-lto -thinlto -o %t/foo %t/foo_fe.bc
+// RUN: llvm-lto -thinlto -o %t/foo_pm %t/foo_fe_pm.bc
+// Ensure Pass PGOInstrumentationGenPass is invoked in PostLink.
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe.bc -fthinlto-index=%t/foo.thinlto.bc -fprofile-instrument=csllvm -fprofile-instrument-path=default.profraw  -flto=thin -emit-llvm -mllvm -debug-pass=Structure -o - 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe_pm.bc -fthinlto-index=%t/foo_pm.thinlto.bc -fexperimental-new-pass-manager -fdebug-pass-manager  -fprofile-instrument=csllvm -fprofile-instrument-path=default.profraw  -flto=thin -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NOT: PGOInstrumentationUsePass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NOT: PGOInstrumentationGenCreateVarPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST: PGOInstrumentationGenPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM-NOT: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM-NOT: Running pass: PGOInstrumentationGenCreateVar
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM: Running pass: PGOInstrumentationGen on
+//
+// RUN: llvm-profdata merge -o %t/cs.profdata %S/Inputs/pgotestir_cs.proftext
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked Once in PreLink.
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/cs.profdata %s -flto=thin -mllvm -debug-pass=Structure -emit-llvm-bc -o %t/foo_fe.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/cs.profdata %s -flto=thin -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm-bc -o %t/foo_fe_pm.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE: PGOInstrumentationUsePass
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NOT: PGOInstrumentationUsePass
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM-NOT: Running pass: PGOInstrumentationUse
+//
+// RUN: llvm-lto -thinlto -o %t/foo %t/foo_fe.bc
+// RUN: llvm-lto -thinlto -o %t/foo_pm %t/foo_fe_pm.bc
+// Ensure Pass PGOInstrumentationUSEPass is invoked in PostLink.
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe.bc -fthinlto-index=%t/foo.thinlto.bc -fprofile-instrument-use-path=%t/cs.profdata -flto=thin -emit-llvm -mllvm -debug-pass=Structure -o - 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-P

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

[Rong Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355331: [PGO] Clang part of change for context-sensitive PGO 
(part1) (authored by xur, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D54176?vs=189167&id=189184#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54176/new/

https://reviews.llvm.org/D54176

Files:
  docs/UsersManual.rst
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Frontend/CompilerInvocation.cpp

Index: docs/UsersManual.rst
===
--- docs/UsersManual.rst
+++ docs/UsersManual.rst
@@ -1745,7 +1745,8 @@
 ``-fprofile-use``. Although these flags are semantically equivalent to
 their GCC counterparts, they *do not* handle GCC-compatible profiles.
 They are only meant to implement GCC's semantics with respect to
-profile creation and use.
+profile creation and use. Flag ``-fcs-profile-generate`` also instruments
+programs using the same instrumentation method as ``-fprofile-generate``.
 
 .. option:: -fprofile-generate[=]
 
@@ -1778,6 +1779,45 @@
  ``LLVM_PROFILE_FILE`` can still be used to override
  the directory and filename for the profile file at runtime.
 
+.. option:: -fcs-profile-generate[=]
+
+  The ``-fcs-profile-generate`` and ``-fcs-profile-generate=`` flags will use
+  the same instrumentation method, and generate the same profile as in the
+  ``-fprofile-generate`` and ``-fprofile-generate=`` flags. The difference is
+  that the instrumentation is performed after inlining so that the resulted
+  profile has a better context sensitive information. They cannot be used
+  together with ``-fprofile-generate`` and ``-fprofile-generate=`` flags.
+  They are typically used in conjunction with ``-fprofile-use`` flag.
+  The profile generated by ``-fcs-profile-generate`` and ``-fprofile-generate``
+  can be merged by llvm-profdata. A use example:
+
+  .. code-block:: console
+
+$ clang++ -O2 -fprofile-generate=yyy/zzz code.cc -o code
+$ ./code
+$ llvm-profdata merge -output=code.profdata yyy/zzz/
+
+  The first few steps are the same as that in ``-fprofile-generate``
+  compilation. Then perform a second round of instrumentation.
+
+  .. code-block:: console
+
+$ clang++ -O2 -fprofile-use=code.profdata -fcs-profile-generate=sss/ttt \
+  -o cs_code
+$ ./cs_code
+$ llvm-profdata merge -output=cs_code.profdata sss/ttt code.profdata
+
+  The resulted ``cs_code.prodata`` combines ``code.profdata`` and the profile
+  generated from binary ``cs_code``. Profile ``cs_code.profata`` can be used by
+  ``-fprofile-use`` compilaton.
+
+  .. code-block:: console
+
+$ clang++ -O2 -fprofile-use=cs_code.profdata
+
+  The above command will read both profiles to the compiler at the identical
+  point of instrumenations.
+
 .. option:: -fprofile-use[=]
 
   Without any other arguments, ``-fprofile-use`` behaves identically to
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -747,6 +747,12 @@
 def fprofile_generate_EQ : Joined<["-"], "fprofile-generate=">,
 Group, Flags<[DriverOption]>, MetaVarName<"">,
 HelpText<"Generate instrumented code to collect execution counts into /default.profraw (overridden by LLVM_PROFILE_FILE env var)">;
+def fcs_profile_generate : Flag<["-"], "fcs-profile-generate">,
+Group, Flags<[DriverOption]>,
+HelpText<"Generate instrumented code to collect context sensitive execution counts into default.profraw (overridden by LLVM_PROFILE_FILE env var)">;
+def fcs_profile_generate_EQ : Joined<["-"], "fcs-profile-generate=">,
+Group, Flags<[DriverOption]>, MetaVarName<"">,
+HelpText<"Generate instrumented code to collect context sensitive execution counts into /default.profraw (overridden by LLVM_PROFILE_FILE env var)">;
 def fprofile_use : Flag<["-"], "fprofile-use">, Group,
 Alias;
 def fprofile_use_EQ : Joined<["-"], "fprofile-use=">,
Index: include/clang/Basic/CodeGenOptions.h
===
--- include/clang/Basic/CodeGenOptions.h
+++ include/clang/Basic/CodeGenOptions.h
@@ -100,6 +100,7 @@
 ProfileClangInstr, // Clang instrumentation to generate execution counts
// to use with PGO.
 ProfileIRInstr,// IR level PGO instrumentation in LLVM.
+ProfileCSIRInstr, // IR level PGO context sensitive instrumentation in LLVM.
   };
 
   enum EmbedBitcodeKind {
@@ -203,8 +204,8 @@
   /// A list of linker options to embed in the object file.
   std::vector LinkerOptions;
 
-  /// Name of the profile file to use as output for -fprofile-instr-generate
-  /// and -fprofile-generate.
+  /// Name of the profi

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

[Rong Xu via Phabricator via cfe-commits]

xur accepted this revision.
xur added a comment.
This revision is now accepted and ready to land.

LGTM.  We need to Initialize the OptLevel no matter what.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61022/new/

https://reviews.llvm.org/D61022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61138: [PGO] Enable InstrProf lowering for Clang PGO instrumentation in the new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur created this revision.
xur added reviewers: vsk, davidxl.
Herald added a subscriber: jfb.

Currently InstrProf lowering is not enabled for Clang PGO instrumentation in 
the new pass manager.
The following command "-fprofile-instr-generate -fexperimental-new-pass-manager 
..." is broken.
This CL enables InstrProf lowering pass for Clang PGO instrumentation in the 
new pass manager.


https://reviews.llvm.org/D61138

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/pgo-instrumentation.c

Index: clang/test/CodeGen/pgo-instrumentation.c
===
--- clang/test/CodeGen/pgo-instrumentation.c
+++ clang/test/CodeGen/pgo-instrumentation.c
@@ -1,20 +1,36 @@
 // Test if PGO instrumentation and use pass are invoked.
 //
 // Ensure Pass PGOInstrumentationGenPass is invoked.
-// RUN: %clang_cc1 -O2 -fprofile-instrument=llvm %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// RUN: %clang_cc1 -O2 -fprofile-instrument=llvm %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN --check-prefix=CHECK-INSTRPROF
+// RUN: %clang_cc1 -O2 -fprofile-instrument=llvm %s  -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-NEWPM --check-prefix=CHECK-INSTRPROF-NEWPM
 // CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass
+// CHECK-INSTRPROF: Frontend instrumentation-based coverage lowering
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-NEWPM: Running pass: PGOInstrumentationGen on
+// CHECK-INSTRPROF-NEWPM: Running pass: InstrProfiling on
 //
 // Ensure Pass PGOInstrumentationGenPass is not invoked.
 // RUN: %clang_cc1 -O2 -fprofile-instrument=clang %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG
+// RUN: %clang_cc1 -O2 -fprofile-instrument=clang %s -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NEWPM
 // CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NOT: PGOInstrumentationGenPass
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NEWPM-NOT: Running pass: PGOInstrumentationGen on
+
+// RUN: %clang_cc1 -O2 -fprofile-instrument=clang %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-CLANG-INSTRPROF
+// RUN: %clang_cc1 -O2 -fprofile-instrument=clang %s -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-CLANG-INSTRPROF-NEWPM
+// RUN: %clang_cc1 -O0 -fprofile-instrument=clang %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-CLANG-INSTRPROF
+// RUN: %clang_cc1 -O0 -fprofile-instrument=clang %s -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-CLANG-INSTRPROF-NEWPM
+// CHECK-CLANG-INSTRPROF: Frontend instrumentation-based coverage lowering
+// CHECK-CLANG-INSTRPROF-NEWPM: Running pass: InstrProfiling on
 
 // Ensure Pass PGOInstrumentationUsePass is invoked.
 // RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotestir.profraw
 // RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t.profdata %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-INSTR-USE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t.profdata %s -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-INSTR-USE-NEWPM
 // CHECK-PGOUSEPASS-INVOKED-INSTR-USE: PGOInstrumentationUsePass
+// CHECK-PGOUSEPASS-INVOKED-INSTR-USE-NEWPM: Running pass: PGOInstrumentationUse on
 //
 // Ensure Pass PGOInstrumentationUsePass is not invoked.
 // RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotestclang.profraw
 // RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t.profdata %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE-CLANG
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t.profdata %s -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE-CLANG-NEWPM
 // CHECK-PGOUSEPASS-INVOKED-USE-CLANG-NOT: PGOInstrumentationUsePass
-
+// CHECK-PGOUSEPASS-INVOKED-USE-CLANG-NEWPM-NOT: Running pass: PGOInstrumentationUse on
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -57,6 +57,7 @@
 #include "llvm/Transforms/Instrumentation/AddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
+#include "llvm/Transforms/Instrumentation/InstrProfiling.h"
 #include "llvm

[PATCH] D61138: [PGO] Enable InstrProf lowering for Clang PGO instrumentation in the new pass manager

[Rong Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC359215: [PGO] Enable InstrProf lowering for Clang PGO 
instrumentation in the new pass… (authored by xur, committed by ).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D61138?vs=196663&id=196683#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61138/new/

https://reviews.llvm.org/D61138

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/pgo-instrumentation.c
  test/Profile/c-general.c

Index: test/Profile/c-general.c
===
--- test/Profile/c-general.c
+++ test/Profile/c-general.c
@@ -1,10 +1,12 @@
 // Test instrumentation of general constructs in C.
 
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument=clang | FileCheck -allow-deprecated-dag-overlap  -check-prefix=PGOGEN %s
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument=clang -fexperimental-new-pass-manager | FileCheck -allow-deprecated-dag-overlap  -check-prefix=PGOGEN %s
 
 // RUN: llvm-profdata merge %S/Inputs/c-general.proftext -o %t.profdata
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata | FileCheck -allow-deprecated-dag-overlap  -check-prefix=PGOUSE %s
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument-use-path=%S/Inputs/c-general.profdata.v3 | FileCheck -allow-deprecated-dag-overlap  -check-prefix=PGOUSE %s
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument-use-path=%S/Inputs/c-general.profdata.v3 -fexperimental-new-pass-manager | FileCheck -allow-deprecated-dag-overlap  -check-prefix=PGOUSE %s
 // Also check compatibility with older profiles.
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument-use-path=%S/Inputs/c-general.profdata.v1 | FileCheck -allow-deprecated-dag-overlap  -check-prefix=PGOUSE %s
 
Index: test/CodeGen/pgo-instrumentation.c
===
--- test/CodeGen/pgo-instrumentation.c
+++ test/CodeGen/pgo-instrumentation.c
@@ -1,20 +1,36 @@
 // Test if PGO instrumentation and use pass are invoked.
 //
 // Ensure Pass PGOInstrumentationGenPass is invoked.
-// RUN: %clang_cc1 -O2 -fprofile-instrument=llvm %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// RUN: %clang_cc1 -O2 -fprofile-instrument=llvm %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN --check-prefix=CHECK-INSTRPROF
+// RUN: %clang_cc1 -O2 -fprofile-instrument=llvm %s  -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-NEWPM --check-prefix=CHECK-INSTRPROF-NEWPM
 // CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass
+// CHECK-INSTRPROF: Frontend instrumentation-based coverage lowering
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-NEWPM: Running pass: PGOInstrumentationGen on
+// CHECK-INSTRPROF-NEWPM: Running pass: InstrProfiling on
 //
 // Ensure Pass PGOInstrumentationGenPass is not invoked.
 // RUN: %clang_cc1 -O2 -fprofile-instrument=clang %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG
+// RUN: %clang_cc1 -O2 -fprofile-instrument=clang %s -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NEWPM
 // CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NOT: PGOInstrumentationGenPass
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NEWPM-NOT: Running pass: PGOInstrumentationGen on
+
+// RUN: %clang_cc1 -O2 -fprofile-instrument=clang %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-CLANG-INSTRPROF
+// RUN: %clang_cc1 -O2 -fprofile-instrument=clang %s -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-CLANG-INSTRPROF-NEWPM
+// RUN: %clang_cc1 -O0 -fprofile-instrument=clang %s -mllvm -debug-pass=Structure -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-CLANG-INSTRPROF
+// RUN: %clang_cc1 -O0 -fprofile-instrument=clang %s -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-CLANG-INSTRPROF-NEWPM
+// CHECK-CLANG-INSTRPROF: Frontend instrumentation-based coverage lowering
+// CHECK-CLANG-INSTRPROF-NEWPM: Running pass: InstrProfiling on
 
 // Ensure Pass PGOInstrumentationUsePass is invoked.
 // RUN: llvm-profdata merge -o %t.profdata %S/

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 186023.
xur added a comment.

Updated the patch to sync with D54175 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54176/new/

https://reviews.llvm.org/D54176

Files:
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotestir.proftext
  test/CodeGen/Inputs/pgotestir_cs.proftext
  test/CodeGen/cspgo-instrumentation.c
  test/CodeGen/cspgo-instrumentation_lto.c
  test/CodeGen/cspgo-instrumentation_thinlto.c

Index: test/CodeGen/cspgo-instrumentation_thinlto.c
===
--- test/CodeGen/cspgo-instrumentation_thinlto.c
+++ test/CodeGen/cspgo-instrumentation_thinlto.c
@@ -0,0 +1,52 @@
+// Test if CSPGO instrumentation and use pass are invoked in thinlto.
+//
+// RUN: rm -rf %t && mkdir %t
+// RUN: llvm-profdata merge -o %t/noncs.profdata %S/Inputs/pgotestir.proftext
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked in PreLink.
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/noncs.profdata -fprofile-instrument=csllvm %s -fprofile-instrument-path=default.profraw -flto=thin -mllvm -debug-pass=Structure -emit-llvm-bc -o %t/foo_fe.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/noncs.profdata -fprofile-instrument=csllvm %s -fprofile-instrument-path=default.profraw  -flto=thin -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm-bc -o %t/foo_fe_pm.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE: PGOInstrumentationUsePass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE: PGOInstrumentationGenCreateVarPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NOT: PGOInstrumentationGenPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM: Running pass: PGOInstrumentationGenCreateVar
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-PRE-NEWPM-NOT: Running pass: PGOInstrumentationGen on
+//
+// RUN: llvm-lto -thinlto -o %t/foo %t/foo_fe.bc
+// RUN: llvm-lto -thinlto -o %t/foo_pm %t/foo_fe_pm.bc
+// Ensure Pass PGOInstrumentationGenPass is invoked in PostLink.
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe.bc -fthinlto-index=%t/foo.thinlto.bc -fprofile-instrument=csllvm -fprofile-instrument-path=default.profraw  -flto=thin -emit-llvm -mllvm -debug-pass=Structure -o - |& FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe_pm.bc -fthinlto-index=%t/foo_pm.thinlto.bc -fexperimental-new-pass-manager -fdebug-pass-manager  -fprofile-instrument=csllvm -fprofile-instrument-path=default.profraw  -flto=thin -emit-llvm -o - |& FileCheck %s -check-prefix=CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NOT: PGOInstrumentationUsePass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NOT: PGOInstrumentationGenCreateVarPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST: PGOInstrumentationGenPass
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM-NOT: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM-NOT: Running pass: PGOInstrumentationGenCreateVar
+// CHECK-CSPGOGENPASS-INVOKED-INSTR-GEN-POST-NEWPM: Running pass: PGOInstrumentationGen on
+//
+// RUN: llvm-profdata merge -o %t/cs.profdata %S/Inputs/pgotestir_cs.proftext
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked Once in PreLink.
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/cs.profdata %s -flto=thin -mllvm -debug-pass=Structure -emit-llvm-bc -o %t/foo_fe.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE
+// RUN: %clang_cc1 -O2 -fprofile-instrument-use-path=%t/cs.profdata %s -flto=thin -fexperimental-new-pass-manager -fdebug-pass-manager -emit-llvm-bc -o %t/foo_fe_pm.bc 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE: PGOInstrumentationUsePass
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NOT: PGOInstrumentationUsePass
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM: Running pass: PGOInstrumentationUse
+// CHECK-CSPGOUSEPASS-INVOKED-INSTR-USE-PRE-NEWPM-NOT: Running pass: PGOInstrumentationUse
+//
+// RUN: llvm-lto -thinlto -o %t/foo %t/foo_fe.bc
+// RUN: llvm-lto -thinlto -o %t/foo_pm %t/foo_fe_pm.bc
+// Ensure Pass PGOInstrumentationUSEPass is invoked in PostLink.
+// RUN: %clang_cc1 -O2 -x ir %t/foo_fe.bc -fthinlto-index=%t/foo.thinlto.bc -fprofile-instrument-use-path=%t/cs.profdata -flto=thin -emit-llvm -mllvm -debug-pass=Structure -o - 2>&1 | FileCheck %s -check-prefix=CHECK-CSPGOUSEPASS-INVO

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur marked 3 inline comments as done.
xur added a comment.

I'm sorry that I missed this review for this long!

In D64029#1581952 , @chandlerc wrote:

> Sorry for the delay here.
>
> It'd be nice to land the LLVM patch first and with its own testing -- we 
> should have testing for the pass builder independent of Clang (IE, in the 
> LLVM tests).
>
> One option would be to test it with a unittest, not sure we have pass builder 
> unittests at the moment.


I initially had the pipeline change for this. But clang did not use the default 
pipeline, so I removed that change.
But this makes sense now to me with a unittest.

> Probably a better option would be to define a pseudo pass name like our 
> `default`, maybe `pgo`. Then you can parse the `O0` the same way we 
> do for the `default` thing. when it is O0 you can call the new routine I've 
> suggested below. When it is higher, you can call the existing routine much 
> like the default pipeline code does. This would all be implemented inside the 
> pass builder so no issues w/ using the utility code there.
> 
> This would let us much more nicely write tests for the pipeline fragment 
> generated for PGO both at O0 and above -- we have tests that specifically 
> print out the pipeline sequence. This makes it very easy to visualize changes 
> to the pipeline. And it would let you test the O0 code path here.

hmm. I think PGO is orthogonal to default pipeline: we now have prefixes of 
"default", "thinlto-pre-link", "thinlto", "lto", "lto-pre-link". They all can 
work with PGO enabled. It seems to me PGO will not cover all of them.

> The clang part of the patch (once adapted to the narrower API) seems likely 
> to be good then.
> 
> I'm happy for this to be two patches (first llvm, then Clang), or one patch. 
> Whatever is easier for you.

Great! I would prefer to have only one patch -- that would be easier.

In D64029#1581952 , @chandlerc wrote:

> Sorry for the delay here.
>
> It'd be nice to land the LLVM patch first and with its own testing -- we 
> should have testing for the pass builder independent of Clang (IE, in the 
> LLVM tests).
>
> One option would be to test it with a unittest, not sure we have pass builder 
> unittests at the moment.
>
> Probably a better option would be to define a pseudo pass name like our 
> `default`, maybe `pgo`. Then you can parse the `O0` the same way we 
> do for the `default` thing. when it is O0 you can call the new routine I've 
> suggested below. When it is higher, you can call the existing routine much 
> like the default pipeline code does. This would all be implemented inside the 
> pass builder so no issues w/ using the utility code there.
>
> This would let us much more nicely write tests for the pipeline fragment 
> generated for PGO both at O0 and above -- we have tests that specifically 
> print out the pipeline sequence. This makes it very easy to visualize changes 
> to the pipeline. And it would let you test the O0 code path here.
>
> The clang part of the patch (once adapted to the narrower API) seems likely 
> to be good then.
>
> I'm happy for this to be two patches (first llvm, then Clang), or one patch. 
> Whatever is easier for you.







Comment at: llvm/lib/Passes/PassBuilder.cpp:576
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
+  }

chandlerc wrote:
> xur wrote:
> > I moved this pass under the condition when Early inline is enabled. I'm 
> > under the impression that the intention is to clean up the code for the 
> > inline.
> > Chandler: you added this pass. If you don't like this move, I can pull it 
> > out and put it under another (Level > 0) condition.
> This makes sense to me. It was probably just transcription error on my part.
Got it.



Comment at: llvm/lib/Passes/PassBuilder.cpp:579-602
   if (RunProfileGen) {
 MPM.addPass(PGOInstrumentationGen(IsCS));
 
-FunctionPassManager FPM;
-FPM.addPass(
-createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+if (Level > 0) {
+  FunctionPassManager FPM;
+  FPM.addPass(
+  createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));

chandlerc wrote:
> Rather than moving the entire routine that is really only intended for the 
> innards of the pass manager to be public, I'd do something a bit more 
> narrow...
> 
> Maybe just add pipeline method to the pass builder to generate a minimal 
> pipeline suitable for O0 usage?
> 
> I think this code will be simpler to read w/o having to have the O0 
> conditions plumbed all the way through it, and the duplication is tiny.
> 
> If you want, could pull out the use bits which are common and use early exit 
> to make the code more clear:
> 
> ```
> if (!RunProfileGen) {
>   addPGOUsePasses(...);
>   return;
> 

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 212237.
xur marked an inline comment as done.
xur added a comment.

Integrated Chandler's review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64029/new/

https://reviews.llvm.org/D64029

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/Profile/Inputs/gcc-flag-compatibility_IR.proftext
  clang/test/Profile/gcc-flag-compatibility.c
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-pgo-O0.ll

Index: llvm/test/Other/new-pm-pgo-O0.ll
===
--- /dev/null
+++ llvm/test/Other/new-pm-pgo-O0.ll
@@ -0,0 +1,21 @@
+; RUN: opt -debug-pass-manager -passes='default' -pgo-kind=pgo-instr-gen-pipeline -profile-file='temp' %s 2>&1 |FileCheck %s --check-prefixes=GEN
+; RUN: llvm-profdata merge %S/Inputs/new-pm-pgo.proftext -o %t.profdata
+; RUN: opt -debug-pass-manager -passes='default' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 |FileCheck %s --check-prefixes=USE_DEFAULT,USE
+; RUN: opt -debug-pass-manager -passes='thinlto-pre-link' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_PRE_LINK,USE
+; RUN: opt -debug-pass-manager -passes='lto-pre-link' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_PRE_LINK,USE
+; RUN: opt -debug-pass-manager -passes='thinlto' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_POST_LINK,USE
+
+;
+; GEN: Running pass: PGOInstrumentationGen
+; USE_DEFAULT: Running pass: PGOInstrumentationUse
+; USE_PRE_LINK: Running pass: PGOInstrumentationUse
+; USE_POST_LINK-NOT: Running pass: PGOInstrumentationUse
+; USE-NOT: Running pass: PGOIndirectCallPromotion
+; USE-NOT: Running pass: PGOMemOPSizeOpt
+
+define void @foo() {
+  ret void
+}
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -541,6 +541,7 @@
 bool RunProfileGen, bool IsCS,
 std::string ProfileFile,
 std::string ProfileRemappingFile) {
+  assert(Level != O0 && "Not expecting O0 here!");
   // Generally running simplification passes and the inliner with an high
   // threshold results in smaller executables, but there may be cases where
   // the size grows, so let's be conservative here and skip this simplification
@@ -571,34 +572,62 @@
 CGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM)));
 
 MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPipeline)));
+
+// Delete anything that is now dead to make sure that we don't instrument
+// dead code. Instrumentation can end up keeping dead code around and
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
   }
 
-  // Delete anything that is now dead to make sure that we don't instrument
-  // dead code. Instrumentation can end up keeping dead code around and
-  // dramatically increase code size.
-  MPM.addPass(GlobalDCEPass());
+  if (!RunProfileGen) {
+assert(!ProfileFile.empty() && "Profile use expecting a profile file!");
+MPM.addPass(PGOInstrumentationUse(ProfileFile, ProfileRemappingFile, IsCS));
+// Cache ProfileSummaryAnalysis once to avoid the potential need to insert
+// RequireAnalysisPass for PSI before subsequent non-module passes.
+MPM.addPass(RequireAnalysisPass());
+return;
+  }
 
-  if (RunProfileGen) {
-MPM.addPass(PGOInstrumentationGen(IsCS));
+  // Perform PGO instrumentation.
+  MPM.addPass(PGOInstrumentationGen(IsCS));
 
-FunctionPassManager FPM;
-FPM.addPass(
-createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
-
-// Add the profile lowering pass.
-InstrProfOptions Options;
-if (!ProfileFile.empty())
-  Options.InstrProfileOutput = ProfileFile;
-Options.DoCounterPromotion = true;
-Options.UseBFIInPromotion = IsCS;
-MPM.addPass(InstrProfiling(Options, IsCS));
-  } else if (!ProfileFile.empty()) {
+  FunctionPassManager FPM;
+  FPM.addPass(createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
+  MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+
+  // Add the profile lowering pass.
+  InstrProfOptions Options;
+  if (!ProfileFile.empty())
+Options.InstrProfileOutput = ProfileFile;
+  // Do counter promotion at Level greater than O0.
+  Options.DoCounterPromotion = true;
+  Options.UseBFIInPromotion = IsCS;
+  MPM.addPass(InstrProfiling(Options, IsCS));
+}
+
+void PassBuilder::addPGOInstrPassesForO0(ModulePassManager &MPM,
+ bool DebugLogging, bool RunProf

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur marked 3 inline comments as done.
xur added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1125
+PB.addPGOInstrPassesForO0(MPM, CodeGenOpts.DebugPassManager,
+  /* RunProfileGen */ PGOOpt->Action ==
+  PGOOptions::IRInstr,

chandlerc wrote:
> Minor nit, here and else where with the comment-named bools I'd use the 
> style: `/*RunProfileGen=*/PGOOpt->Action == PGOOPtions::IRInstr`
> 
> This was suggested as more consistent w/ Clang style, and it seems a bit 
> cleaner. Also, clang-tidy will recognize and check that the comment uses the 
> same name as the parameter.
Thanks for the suggestion and the reasoning.

I actually wrote it in one line.
This was produced by "clang/tools/clang-format/clang-format-diff.py". :-)

I'm using now "/*RunProfileGen=*/ (PGOOpt->Action == PGOOPtions::IRInstr)" so 
clang-format-diff.py won't complain



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64029/new/

https://reviews.llvm.org/D64029



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
xur marked an inline comment as done.
Closed by commit rL367628: [PGO] Add PGO support at -O0 in the experimental new 
pass manager (authored by xur, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D64029?vs=212237&id=212918#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64029/new/

https://reviews.llvm.org/D64029

Files:
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/test/Profile/Inputs/gcc-flag-compatibility_IR.proftext
  cfe/trunk/test/Profile/gcc-flag-compatibility.c
  llvm/trunk/include/llvm/Passes/PassBuilder.h
  llvm/trunk/lib/Passes/PassBuilder.cpp
  llvm/trunk/test/Other/new-pm-pgo-O0.ll

Index: llvm/trunk/include/llvm/Passes/PassBuilder.h
===
--- llvm/trunk/include/llvm/Passes/PassBuilder.h
+++ llvm/trunk/include/llvm/Passes/PassBuilder.h
@@ -629,6 +629,12 @@
 TopLevelPipelineParsingCallbacks.push_back(C);
   }
 
+  /// Add PGOInstrumenation passes for O0 only.
+  void addPGOInstrPassesForO0(ModulePassManager &MPM, bool DebugLogging,
+  bool RunProfileGen, bool IsCS,
+  std::string ProfileFile,
+  std::string ProfileRemappingFile);
+
 private:
   static Optional>
   parsePipelineText(StringRef Text);
@@ -660,7 +666,6 @@
  OptimizationLevel Level, bool RunProfileGen, bool IsCS,
  std::string ProfileFile,
  std::string ProfileRemappingFile);
-
   void invokePeepholeEPCallbacks(FunctionPassManager &, OptimizationLevel);
 
   // Extension Point callbacks
Index: llvm/trunk/test/Other/new-pm-pgo-O0.ll
===
--- llvm/trunk/test/Other/new-pm-pgo-O0.ll
+++ llvm/trunk/test/Other/new-pm-pgo-O0.ll
@@ -0,0 +1,21 @@
+; RUN: opt -debug-pass-manager -passes='default' -pgo-kind=pgo-instr-gen-pipeline -profile-file='temp' %s 2>&1 |FileCheck %s --check-prefixes=GEN
+; RUN: llvm-profdata merge %S/Inputs/new-pm-pgo.proftext -o %t.profdata
+; RUN: opt -debug-pass-manager -passes='default' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 |FileCheck %s --check-prefixes=USE_DEFAULT,USE
+; RUN: opt -debug-pass-manager -passes='thinlto-pre-link' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_PRE_LINK,USE
+; RUN: opt -debug-pass-manager -passes='lto-pre-link' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_PRE_LINK,USE
+; RUN: opt -debug-pass-manager -passes='thinlto' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_POST_LINK,USE
+
+;
+; GEN: Running pass: PGOInstrumentationGen
+; USE_DEFAULT: Running pass: PGOInstrumentationUse
+; USE_PRE_LINK: Running pass: PGOInstrumentationUse
+; USE_POST_LINK-NOT: Running pass: PGOInstrumentationUse
+; USE-NOT: Running pass: PGOIndirectCallPromotion
+; USE-NOT: Running pass: PGOMemOPSizeOpt
+
+define void @foo() {
+  ret void
+}
Index: llvm/trunk/lib/Passes/PassBuilder.cpp
===
--- llvm/trunk/lib/Passes/PassBuilder.cpp
+++ llvm/trunk/lib/Passes/PassBuilder.cpp
@@ -541,6 +541,7 @@
 bool RunProfileGen, bool IsCS,
 std::string ProfileFile,
 std::string ProfileRemappingFile) {
+  assert(Level != O0 && "Not expecting O0 here!");
   // Generally running simplification passes and the inliner with an high
   // threshold results in smaller executables, but there may be cases where
   // the size grows, so let's be conservative here and skip this simplification
@@ -571,34 +572,62 @@
 CGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM)));
 
 MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPipeline)));
+
+// Delete anything that is now dead to make sure that we don't instrument
+// dead code. Instrumentation can end up keeping dead code around and
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
   }
 
-  // Delete anything that is now dead to make sure that we don't instrument
-  // dead code. Instrumentation can end up keeping dead code around and
-  // dramatically increase code size.
-  MPM.addPass(GlobalDCEPass());
+  if (!RunProfileGen) {
+assert(!ProfileFile.empty() && "Profile use expecting a profile file!");
+MPM.addPass(PGOInstrumentationUse(ProfileFile, ProfileRemappingFile, IsCS));
+// Cache ProfileSummaryAnalysis once to avoid the potential need to insert
+// RequireAnalysisPass for PSI before subsequent non-module passes.
+MPM.addPass(RequireAnalysisPass());
+return;
+  }
 
-  if 

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't 
do PGO instrumentation at -O0. This is due to the fact that PGO 
instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass
 MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
which only gives you the wrong signal  of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions 
for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63155/new/

https://reviews.llvm.org/D63155



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

they are not doing the exacly the same thing for old pass manager and new pass 
manger: old pass manger is doing instrumentation, while the new pass manager 
with this change is NOT.
the test is not check instrumentation, (it only check the variables that 
generates by InstroProfiling pass). 
In this sense, the test is not well written.

I can draft a patch for this.

In D63155#1563239 , @chandlerc wrote:

> In D63155#1563229 , @xur wrote:
>
> > This patch does not make sense to me.
> >
> > The reason of failing with -fexperimental-new-pass-manager is because we 
> > don't do PGO instrumentation at -O0. This is due to the fact that PGO 
> > instrumentation/use passes are under 
> > PassBuilder::buildPerModuleDefaultPipeline.
> >
> > This patch add a pass
> >
> >   MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
> >
> > which only gives you the wrong signal  of instrumentation is done.
> >
> > I wrote pass PGOInstrumentationGenCreateVar only for some trick 
> > interactions for thinlto under ldd for CSPGO.
> >  Regular FDO should not use it.
> >
> > The right fix is to enable PGO instrumentation and use in pass builder for 
> > O0.
> >
> > I would like to request to revert this patch.
>
>
> I mean, I'm happy for the patch to be reverted, but I still really don't 
> understand why this fixes the test to work *exactly* the same as w/ the old 
> pass manager and doesn't break any other tests if it is completely wrong? It 
> seems like there must be something strange with the test coverage if this is 
> so far off of correct without any failures...
>
> I also don't understand what fix you are suggesting instead, but maybe you 
> can show a patch?




In D63155#1563266 , @chandlerc wrote:

> In D63155#1563240 , @leonardchan 
> wrote:
>
> > In D63155#1563229 , @xur wrote:
> >
> > > This patch does not make sense to me.
> > >
> > > The reason of failing with -fexperimental-new-pass-manager is because we 
> > > don't do PGO instrumentation at -O0. This is due to the fact that PGO 
> > > instrumentation/use passes are under 
> > > PassBuilder::buildPerModuleDefaultPipeline.
> > >
> > > This patch add a pass
> > >
> > >   MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
> > >
> > > which only gives you the wrong signal  of instrumentation is done.
> > >
> > > I wrote pass PGOInstrumentationGenCreateVar only for some trick 
> > > interactions for thinlto under ldd for CSPGO.
> > >  Regular FDO should not use it.
> > >
> > > The right fix is to enable PGO instrumentation and use in pass builder 
> > > for O0.
> > >
> > > I would like to request to revert this patch.
> >
> >
> > As an alternative, could I instead request that we remove the BackendUtil 
> > changes and just mark the runs in gcc-flag-compatibility.c with 
> > `-fno-experimental-new-pass-manager`. This way we could clarify that under 
> > the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as 
> > is.
>
>
> No, I think we should be doing PGO at O0 in the new PM if we do so in the old 
> PM.
>
> I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct 
> to fix this test case. That seems plausible to me at least, and I think 
> reverting and figuring out what the right way to do it is a fine approach.
>
> I just think we also need to understand why *no other test failed*, and why 
> the only test we have for doing PGO at O0 is this one and it could be "fixed" 
> but such a deeply unrelated change





Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63155/new/

https://reviews.llvm.org/D63155



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

In D63155#1563266 , @chandlerc wrote:

> In D63155#1563240 , @leonardchan 
> wrote:
>
> > In D63155#1563229 , @xur wrote:
> >
> > > This patch does not make sense to me.
> > >
> > > The reason of failing with -fexperimental-new-pass-manager is because we 
> > > don't do PGO instrumentation at -O0. This is due to the fact that PGO 
> > > instrumentation/use passes are under 
> > > PassBuilder::buildPerModuleDefaultPipeline.
> > >
> > > This patch add a pass
> > >
> > >   MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
> > >
> > > which only gives you the wrong signal  of instrumentation is done.
> > >
> > > I wrote pass PGOInstrumentationGenCreateVar only for some trick 
> > > interactions for thinlto under ldd for CSPGO.
> > >  Regular FDO should not use it.
> > >
> > > The right fix is to enable PGO instrumentation and use in pass builder 
> > > for O0.
> > >
> > > I would like to request to revert this patch.
> >
> >
> > As an alternative, could I instead request that we remove the BackendUtil 
> > changes and just mark the runs in gcc-flag-compatibility.c with 
> > `-fno-experimental-new-pass-manager`. This way we could clarify that under 
> > the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as 
> > is.
>
>
> No, I think we should be doing PGO at O0 in the new PM if we do so in the old 
> PM.
>
> I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct 
> to fix this test case. That seems plausible to me at least, and I think 
> reverting and figuring out what the right way to do it is a fine approach.


Just check the IR, we still not doing instrumentation at O0. This patch just 
mask the error.

> I just think we also need to understand why *no other test failed*, and why 
> the only test we have for doing PGO at O0 is this one and it could be "fixed" 
> but such a deeply unrelated change

We have special code to do PGO at O0 in old pass manager. This is not done in 
the new pass manager.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63155/new/

https://reviews.llvm.org/D63155



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur created this revision.
xur added a reviewer: chandlerc.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

Add PGO support at -O0 in the experimental new pass manager to sync the 
behavior with the legacy pass manager.

Also change the test of gcc-flag-compatibility.c for more complete test:
(1) change the match string to "profc" and "profd" to ensure the 
instrumentation is happening.
(2) add IR format proftext so that PGO use is tested.


https://reviews.llvm.org/D64029

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/Profile/Inputs/gcc-flag-compatibility_IR.proftext
  clang/test/Profile/gcc-flag-compatibility.c
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp

Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -544,7 +544,7 @@
   // the size grows, so let's be conservative here and skip this simplification
   // at -Os/Oz. We will not do this  inline for context sensistive PGO (when
   // IsCS is true).
-  if (!isOptimizingForSize(Level) && !IsCS) {
+  if (Level > 0 && !isOptimizingForSize(Level) && !IsCS) {
 InlineParams IP;
 
 // In the old pass manager, this is a cl::opt. Should still this be one?
@@ -569,26 +569,29 @@
 CGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM)));
 
 MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPipeline)));
-  }
 
-  // Delete anything that is now dead to make sure that we don't instrument
-  // dead code. Instrumentation can end up keeping dead code around and
-  // dramatically increase code size.
-  MPM.addPass(GlobalDCEPass());
+// Delete anything that is now dead to make sure that we don't instrument
+// dead code. Instrumentation can end up keeping dead code around and
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
+  }
 
   if (RunProfileGen) {
 MPM.addPass(PGOInstrumentationGen(IsCS));
 
-FunctionPassManager FPM;
-FPM.addPass(
-createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+if (Level > 0) {
+  FunctionPassManager FPM;
+  FPM.addPass(
+  createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
+  MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+}
 
 // Add the profile lowering pass.
 InstrProfOptions Options;
 if (!ProfileFile.empty())
   Options.InstrProfileOutput = ProfileFile;
-Options.DoCounterPromotion = true;
+// Do counter promotion at Level greater than O0.
+Options.DoCounterPromotion = (Level > 0);
 Options.UseBFIInPromotion = IsCS;
 MPM.addPass(InstrProfiling(Options, IsCS));
   } else if (!ProfileFile.empty()) {
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -629,6 +629,12 @@
 TopLevelPipelineParsingCallbacks.push_back(C);
   }
 
+  /// Add PGOInstrumenation passes.
+  void addPGOInstrPasses(ModulePassManager &MPM, bool DebugLogging,
+ OptimizationLevel Level, bool RunProfileGen, bool IsCS,
+ std::string ProfileFile,
+ std::string ProfileRemappingFile);
+
 private:
   static Optional>
   parsePipelineText(StringRef Text);
@@ -656,11 +662,6 @@
 ArrayRef Pipeline,
 bool VerifyEachPass, bool DebugLogging);
 
-  void addPGOInstrPasses(ModulePassManager &MPM, bool DebugLogging,
- OptimizationLevel Level, bool RunProfileGen, bool IsCS,
- std::string ProfileFile,
- std::string ProfileRemappingFile);
-
   void invokePeepholeEPCallbacks(FunctionPassManager &, OptimizationLevel);
 
   // Extension Point callbacks
Index: clang/test/Profile/gcc-flag-compatibility.c
===
--- clang/test/Profile/gcc-flag-compatibility.c
+++ clang/test/Profile/gcc-flag-compatibility.c
@@ -7,26 +7,49 @@
 // -fprofile-use=Uses the profile file /default.profdata
 // -fprofile-use=/file   Uses the profile file /file
 
-// RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate | FileCheck -check-prefix=PROFILE-GEN %s
-// PROFILE-GEN: __llvm_profile_filename
+// RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate -fno-experimental-new-pass-manager | FileCheck -check-prefix=PROFILE-GEN %s
+// RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate -fexperimental-new-pass-manager | FileCheck -check-prefix=PROFILE-GEN %s
+// PROFILE-GEN: @__profc_main = private global [2 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
+// PROFILE-GEN: @__profd_main = private global
 
 // Check that -fprof

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 207392.
xur marked an inline comment as done.
xur added a comment.

Sent the wrong test file in last patch.
Update to the correct one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64029/new/

https://reviews.llvm.org/D64029

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/Profile/Inputs/gcc-flag-compatibility_IR.proftext
  clang/test/Profile/gcc-flag-compatibility.c
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp

Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -544,7 +544,7 @@
   // the size grows, so let's be conservative here and skip this simplification
   // at -Os/Oz. We will not do this  inline for context sensistive PGO (when
   // IsCS is true).
-  if (!isOptimizingForSize(Level) && !IsCS) {
+  if (Level > 0 && !isOptimizingForSize(Level) && !IsCS) {
 InlineParams IP;
 
 // In the old pass manager, this is a cl::opt. Should still this be one?
@@ -569,26 +569,29 @@
 CGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM)));
 
 MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPipeline)));
-  }
 
-  // Delete anything that is now dead to make sure that we don't instrument
-  // dead code. Instrumentation can end up keeping dead code around and
-  // dramatically increase code size.
-  MPM.addPass(GlobalDCEPass());
+// Delete anything that is now dead to make sure that we don't instrument
+// dead code. Instrumentation can end up keeping dead code around and
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
+  }
 
   if (RunProfileGen) {
 MPM.addPass(PGOInstrumentationGen(IsCS));
 
-FunctionPassManager FPM;
-FPM.addPass(
-createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+if (Level > 0) {
+  FunctionPassManager FPM;
+  FPM.addPass(
+  createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
+  MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+}
 
 // Add the profile lowering pass.
 InstrProfOptions Options;
 if (!ProfileFile.empty())
   Options.InstrProfileOutput = ProfileFile;
-Options.DoCounterPromotion = true;
+// Do counter promotion at Level greater than O0.
+Options.DoCounterPromotion = (Level > 0);
 Options.UseBFIInPromotion = IsCS;
 MPM.addPass(InstrProfiling(Options, IsCS));
   } else if (!ProfileFile.empty()) {
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -629,6 +629,12 @@
 TopLevelPipelineParsingCallbacks.push_back(C);
   }
 
+  /// Add PGOInstrumenation passes.
+  void addPGOInstrPasses(ModulePassManager &MPM, bool DebugLogging,
+ OptimizationLevel Level, bool RunProfileGen, bool IsCS,
+ std::string ProfileFile,
+ std::string ProfileRemappingFile);
+
 private:
   static Optional>
   parsePipelineText(StringRef Text);
@@ -656,11 +662,6 @@
 ArrayRef Pipeline,
 bool VerifyEachPass, bool DebugLogging);
 
-  void addPGOInstrPasses(ModulePassManager &MPM, bool DebugLogging,
- OptimizationLevel Level, bool RunProfileGen, bool IsCS,
- std::string ProfileFile,
- std::string ProfileRemappingFile);
-
   void invokePeepholeEPCallbacks(FunctionPassManager &, OptimizationLevel);
 
   // Extension Point callbacks
Index: clang/test/Profile/gcc-flag-compatibility.c
===
--- clang/test/Profile/gcc-flag-compatibility.c
+++ clang/test/Profile/gcc-flag-compatibility.c
@@ -7,26 +7,50 @@
 // -fprofile-use=Uses the profile file /default.profdata
 // -fprofile-use=/file   Uses the profile file /file
 
-// RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate | FileCheck -check-prefix=PROFILE-GEN %s
-// PROFILE-GEN: __llvm_profile_filename
+// RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate -fno-experimental-new-pass-manager | FileCheck -check-prefix=PROFILE-GEN %s
+// RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate -fexperimental-new-pass-manager | FileCheck -check-prefix=PROFILE-GEN %s
+// PROFILE-GEN: @__profc_main = private global [2 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
+// PROFILE-GEN: @__profd_main = private global
 
 // Check that -fprofile-generate=/path/to generates /path/to/default.profraw
-// RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate=/path/to | FileCheck -check-prefix=PROFILE-GEN-EQ %s
+// RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate=

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

Ping




Comment at: llvm/lib/Passes/PassBuilder.cpp:576
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
+  }

I moved this pass under the condition when Early inline is enabled. I'm under 
the impression that the intention is to clean up the code for the inline.
Chandler: you added this pass. If you don't like this move, I can pull it out 
and put it under another (Level > 0) condition.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64029/new/

https://reviews.llvm.org/D64029



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84261: [PGO] Supporting code for always instrumenting entry block

[Rong Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG50da55a58534: [PGO] Supporting code for always instrumenting 
entry block (authored by xur).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D84261?vs=279904&id=279952#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84261/new/

https://reviews.llvm.org/D84261

Files:
  clang/test/CodeGenCXX/Inputs/profile-remap_entry.proftext
  clang/test/CodeGenCXX/profile-remap.cpp
  clang/test/Profile/Inputs/gcc-flag-compatibility_IR_entry.proftext
  clang/test/Profile/gcc-flag-compatibility.c
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfData.inc
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp
  llvm/lib/Transforms/Instrumentation/CFGMST.h
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/test/Transforms/PGOProfile/Inputs/branch2_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/criticaledge_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/func_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/indirectbr_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/irreducible_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/landingpad_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/loop1_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/loop2_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch_entry.proftext
  llvm/test/Transforms/PGOProfile/Inputs/select1.proftext
  llvm/test/Transforms/PGOProfile/Inputs/select2.proftext
  llvm/test/Transforms/PGOProfile/Inputs/switch_entry.proftext
  llvm/test/Transforms/PGOProfile/branch2.ll
  llvm/test/Transforms/PGOProfile/counter_promo_exit_catchswitch.ll
  llvm/test/Transforms/PGOProfile/criticaledge.ll
  llvm/test/Transforms/PGOProfile/cspgo_profile_summary.ll
  llvm/test/Transforms/PGOProfile/indirectbr.ll
  llvm/test/Transforms/PGOProfile/irreducible.ll
  llvm/test/Transforms/PGOProfile/landingpad.ll
  llvm/test/Transforms/PGOProfile/loop1.ll
  llvm/test/Transforms/PGOProfile/loop2.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll
  llvm/test/Transforms/PGOProfile/switch.ll
  llvm/test/Transforms/PGOProfile/thinlto_cspgo_use.ll
  llvm/test/tools/llvm-profdata/Inputs/header-directives-1.proftext
  llvm/test/tools/llvm-profdata/Inputs/header-directives-2.proftext
  llvm/test/tools/llvm-profdata/Inputs/header-directives-3.proftext
  llvm/test/tools/llvm-profdata/header-directives.test
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -251,6 +251,7 @@
 Filename);
 return;
   }
+  WC->Writer.setInstrEntryBBEnabled(Reader->instrEntryBBEnabled());
 
   for (auto &I : *Reader) {
 if (Remapper)
@@ -977,8 +978,11 @@
   if (TextFormat)
 return 0;
   std::unique_ptr PS(Builder.getSummary());
-  OS << "Instrumentation level: "
- << (Reader->isIRLevelProfile() ? "IR" : "Front-end") << "\n";
+  bool IsIR = Reader->isIRLevelProfile();
+  OS << "Instrumentation level: " << (IsIR ? "IR" : "Front-end");
+  if (IsIR)
+OS << "  entry_first = " << Reader->instrEntryBBEnabled();
+  OS << "\n";
   if (ShowAllFunctions || !ShowFunction.empty())
 OS << "Functions shown: " << ShownFunctions << "\n";
   OS << "Total functions: " << PS->getNumFunctions() << "\n";
Index: llvm/test/tools/llvm-profdata/header-directives.test
===
--- /dev/null
+++ llvm/test/tools/llvm-profdata/header-directives.test
@@ -0,0 +1,10 @@
+RUN: llvm-profdata show %p/Inputs/header-directives-1.proftext -o - | FileCheck %s --check-prefixes=ENTRYFIRST,CHECK
+RUN: llvm-profdata show %p/Inputs/header-directives-2.proftext -o - | FileCheck %s --check-prefixes=NOTENTRYFIRST,CHECK
+RUN: llvm-profdata show %p/Inputs/header-directives-3.proftext -o - | FileCheck %s --check-prefixes=ENTRYFIRST,CHECK
+
+ENTRYFIRST: Instrumentation level: IR  entry_first = 1
+NOTENTRYFIRST: Instrumentation level: IR  entry_first = 0
+CHECK: Total functions: 1
+CHECK: Maximum function count: 100
+CHECK: Maximum internal block count: 90
+
Index: llvm/test/tools/llvm-profdata/Inputs/header-directives-3.proftext

[PATCH] D131306: [llvm][misexpect] Track provenance of branch weights

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

I agreed with Teresa: adding an optional string is better than using a 
separated metadata here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131306/new/

https://reviews.llvm.org/D131306

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

[Rong Xu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdb18f26567be: [llvm-profdata] Handle internal linkage 
functions in profile supplementation (authored by xur).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D132600?vs=455816&id=456490#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132600/new/

https://reviews.llvm.org/D132600

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/ProfileData/SampleProf.h
  llvm/test/tools/llvm-profdata/Inputs/FUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/FUnique.proftext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.proftext
  llvm/test/tools/llvm-profdata/suppl-instr-with-sample-static-func.test
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/MD5.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ThreadPool.h"
@@ -42,6 +43,10 @@
 
 using namespace llvm;
 
+// We use this string to indicate that there are
+// multiple static functions map to the same name.
+const std::string DuplicateNameStr = "";
+
 enum ProfileFormat {
   PF_None = 0,
   PF_Text,
@@ -480,7 +485,7 @@
   NumEdgeCounters = CntNum;
 }
 
-/// Either set all the counters in the instr profile entry \p IFE to -1
+// Either set all the counters in the instr profile entry \p IFE to -1
 /// in order to drop the profile or scale up the counters in \p IFP to
 /// be above hot threshold. We use the ratio of zero counters in the
 /// profile of a function to decide the profile is helpful or harmful
@@ -541,7 +546,73 @@
unsigned InstrProfColdThreshold) {
   // Function to its entry in instr profile.
   StringMap InstrProfileMap;
+  StringMap StaticFuncMap;
   InstrProfSummaryBuilder IPBuilder(ProfileSummaryBuilder::DefaultCutoffs);
+
+  auto checkSampleProfileHasFUnique = [&Reader]() {
+for (const auto &PD : Reader->getProfiles()) {
+  auto &FContext = PD.first;
+  if (FContext.toString().find(FunctionSamples::UniqSuffix) !=
+  std::string::npos) {
+return true;
+  }
+}
+return false;
+  };
+
+  bool SampleProfileHasFUnique = checkSampleProfileHasFUnique();
+
+  auto buildStaticFuncMap = [&StaticFuncMap,
+ SampleProfileHasFUnique](const StringRef Name) {
+std::string Prefixes[] = {".cpp:", "cc:", ".c:", ".hpp:", ".h:"};
+size_t PrefixPos = StringRef::npos;
+for (auto &Prefix : Prefixes) {
+  PrefixPos = Name.find_insensitive(Prefix);
+  if (PrefixPos == StringRef::npos)
+continue;
+  PrefixPos += Prefix.size();
+  break;
+}
+
+if (PrefixPos == StringRef::npos) {
+  return;
+}
+
+StringRef NewName = Name.drop_front(PrefixPos);
+StringRef FName = Name.substr(0, PrefixPos - 1);
+if (NewName.size() == 0) {
+  return;
+}
+
+// This name should have a static linkage.
+size_t PostfixPos = NewName.find(FunctionSamples::UniqSuffix);
+bool ProfileHasFUnique = (PostfixPos != StringRef::npos);
+
+// If sample profile and instrumented profile do not agree on symbol
+// uniqification.
+if (SampleProfileHasFUnique != ProfileHasFUnique) {
+  // If instrumented profile uses -funique-internal-linakge-symbols,
+  // we need to trim the name.
+  if (ProfileHasFUnique) {
+NewName = NewName.substr(0, PostfixPos);
+  } else {
+// If sample profile uses -funique-internal-linakge-symbols,
+// we build the map.
+std::string NStr =
+NewName.str() + getUniqueInternalLinkagePostfix(FName);
+NewName = StringRef(NStr);
+StaticFuncMap[NewName] = Name;
+return;
+  }
+}
+
+if (StaticFuncMap.find(NewName) == StaticFuncMap.end()) {
+  StaticFuncMap[NewName] = Name;
+} else {
+  StaticFuncMap[NewName] = DuplicateNameStr;
+}
+  };
+
   for (auto &PD : WC->Writer.getProfileData()) {
 // Populate IPBuilder.
 for (const auto &PDV : PD.getValue()) {
@@ -555,7 +626,9 @@
 
 // Initialize InstrProfileMap.
 InstrProfRecord *R = &PD.getValue().begin()->second;
-InstrProfileMap[PD.getKey()] = InstrProfileEntry(R);
+StringRef FullName = PD.getKey();
+InstrProfileMap[FullName] = InstrProfileEntry(R);
+buildStaticFuncMap(FullName);
   }
 
   ProfileSummary InstrPS = *IPBuilder.getSummary();
@@ -583,16 +656,28 @@
   // Find hot/warm functions in sample profile which is cold i

[PATCH] D132991: [Clang] Give error message for invalid profile path when compiling IR

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

I'm fine with this change.




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1301
+unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
+"Could not read profile %0: %1");
+llvm::handleAllErrors(std::move(E), [&](const llvm::ErrorInfoBase &EI) {

Should we use "Error in reading profile ..."?
Reader still can return error even if the profile can be read.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132991/new/

https://reviews.llvm.org/D132991

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72523: [remark][diagnostics] Using clang diagnostic handler for IR input files

[Rong Xu via Phabricator via cfe-commits]

xur created this revision.
xur added a reviewer: tejohnson.
Herald added subscribers: dexonsmith, steven_wu, hiraditya, mehdi_amini.

For IR input files, we currently use LLVM diagnostic handler even the
compilation is from clang. As a result, we are not able to use -Rpass
to get the transformation reports. Some warnings are not handled
properly either: We found many mysterious warnings in our ThinLTO backend
compilations in SamplePGO and CSPGO. An example of the warning:

warning: net/proto2/public/metadata_lite.h:51:21: 0.02% (1 / 4999)

This turns out to be a warning by Wmisexpect, which is supposed to be
filtered out by default. But since the filter is in clang's
diagnostic hander, we emit these incomplete warnings from LLVM's
diagnostic handler.

This patch uses clang diagnostic handler for IR input files. We create
a fake backendconsumer just to install the diagnostic handler.
The location information is tightly coupled with the sourcemanager which
is not available in IR input files. I just prepend the location from DebugLoc
to the warning/remark messages. I think this is much simpler way than
to pass DebugLoc to DiagnosticEngine (I tried this approach, but it seemed to
require much more extensive changes).

With this change, we will have proper handling of all the warnings and we can
use -Rpass* options in IR input files compilation.
Also note that with is patch, LLVM's diagnostic options, like
"-mllvm -pass-remarks=*", are no longer be able to get optimization remarks.


https://reviews.llvm.org/D72523

Files:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/test/CodeGen/Inputs/thinlto_expect1.proftext
  clang/test/CodeGen/Inputs/thinlto_expect2.proftext
  clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
  clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll

Index: clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
===
--- clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
+++ clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
@@ -28,7 +28,7 @@
 ; YAML-NEXT: ...
 
 ; Next try with pass remarks to stderr
-; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -mllvm -pass-remarks=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
+; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -Rpass=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
 
 ; CHECK: tinkywinky inlined into main with (cost=0, threshold=337) (hotness: 300)
 
Index: clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
===
--- /dev/null
+++ clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
@@ -0,0 +1,22 @@
+// Test clang diagnositic handler works in IR file compilaiton.
+
+// REQUIRES: x86-registered-target
+
+// RUN: llvm-profdata merge -o %t1.profdata %S/Inputs/thinlto_expect1.proftext
+// RUN: %clang -O2 -fexperimental-new-pass-manager -flto=thin -g -fprofile-use=%t1.profdata -c -o %t1.bo %s
+// RUN: llvm-lto -thinlto -o %t %t1.bo
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -emit-obj -Rpass-analysis=info 2>&1 | FileCheck %s -check-prefix=CHECK-REMARK
+// RUN: llvm-profdata merge -o %t2.profdata %S/Inputs/thinlto_expect2.proftext
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj -Wmisexpect 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
+
+int sum;
+__attribute__((noinline)) void bar() {
+  sum = 1234;
+}
+
+__attribute__((noinline)) void foo(int m) {
+  if (__builtin_expect(m > 9, 1))
+bar();
+}
+// CHECK-REMARK: remark: In {{.*}}.c:
+// CHECK-WARNING: warning: Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24) of profiled executions.
Index: clang/test/CodeGen/Inputs/thinlto_expect2.proftext
===
--- /dev/null
+++ clang/test/CodeGen/Inputs/thinlto_expect2.proftext
@@ -0,0 +1,20 @@
+# CSIR level Instrumentation Flag
+:csir
+foo
+# Func Hash:
+25571299074
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
+foo
+# Func Hash:
+1152921530178146050
+# Num Counters:
+2
+# Counter Values:
+24
+12
+
Index: clang/test/CodeGen/Inputs/thinlto_expect1.proftext
===
--- /dev/null
+++ clang/test/CodeGen/Inputs/thinlto_expect1.proftext
@@ -0,0 +1,11 @@
+# IR level Instrumentation Flag
+:ir
+foo
+# Func Hash:
+25571299074
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -151,6 +151,25 @

[PATCH] D72523: [remark][diagnostics] Using clang diagnostic handler for IR input files

[Rong Xu via Phabricator via cfe-commits]

xur marked 3 inline comments as done.
xur added inline comments.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:594
+  // SourceLoc.
+  if (Context == nullptr) {
+return FullSourceLoc();

tejohnson wrote:
> Does this only happen with IR input? Does it always happen with IR input? If 
> not, what happens when we have a non-null Context but IR input? It sounds 
> like it should still always return an invalid Loc since there is no 
> SourceManager? In that case can you set a flag on the BackendConsumer so we 
> can bail out directly in the IR input case, instead of going through the 
> motions only to get an invalid Loc back anyway? It would also be more clear.
Before this patch, IR input does not called to this function. IR input has a 
null Context (so it will seg-fault at the dereference at line 598). Context == 
nullptr should be only happening with the empty BackendConsume that installed 
in this patch. 
I tried to set Context but the whole point was to get SrouceManager which was 
not available for IR input.





Comment at: clang/lib/CodeGen/CodeGenAction.cpp:650
+  if (!Loc.isValid()) {
+MsgStream << D.getLocationStr() << ": in function "
+  << D.getFunction().getName() << ' '

tejohnson wrote:
> Can you add a test for this one?
> 
> Also, what determines the format of the message when Loc is valid? I was 
> trying to figure out where that got determined, but couldn't easily track it 
> down. 
Do you mean adding a test to test the path of branch starting line 650?
(For the case of valid Loc, I don't think we need to test as it's not changed.)

If so, there is already a test for this:
clang/test/CodeGen/backend-unsupported-error.ll
I think this calls to llvm's diagnostic handler for IR input files (before this 
patch). The format is here:
DiagnosticInfoUnsupported::print(...) in  llvm/lib/IR/DiagnosticInfo.cpp

For the case of input other than IR files, the Diags should go to clang 
diagnostic handler.  This patch might change the behavior. 

I think I will check Context == nullptr directly (rather Loc.isValid). This way 
we don't need to call into getBestLocationFromDebugLoc(), and it will not 
affect non-IR input files.




Comment at: clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c:8
+// RUN: llvm-lto -thinlto -o %t %t1.bo
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo 
-fthinlto-index=%t.thinlto.bc -emit-obj -Rpass-analysis=info 2>&1 | FileCheck 
%s -check-prefix=CHECK-REMARK
+// RUN: llvm-profdata merge -o %t2.profdata %S/Inputs/thinlto_expect2.proftext

tejohnson wrote:
> In this case (since no -Wmisexpect), presumably we should not be getting the 
> warning, whereas without your fix we were, correct? In that case, please add 
> a NOT check to confirm that we don't get the message anymore.
Yes. You are right. 
I will add a NOT check to confirm this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72523/new/

https://reviews.llvm.org/D72523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72523: [remark][diagnostics] Using clang diagnostic handler for IR input files

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 237716.
xur added a comment.

Integrated Teresa's review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72523/new/

https://reviews.llvm.org/D72523

Files:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/test/CodeGen/Inputs/thinlto_expect1.proftext
  clang/test/CodeGen/Inputs/thinlto_expect2.proftext
  clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
  clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll

Index: clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
===
--- clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
+++ clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
@@ -28,7 +28,7 @@
 ; YAML-NEXT: ...
 
 ; Next try with pass remarks to stderr
-; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -mllvm -pass-remarks=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
+; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -Rpass=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
 
 ; CHECK: tinkywinky inlined into main with (cost=0, threshold=337) (hotness: 300)
 
Index: clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
===
--- /dev/null
+++ clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
@@ -0,0 +1,24 @@
+// Test clang diagnositic handler works in IR file compilaiton.
+
+// REQUIRES: x86-registered-target
+
+// RUN: llvm-profdata merge -o %t1.profdata %S/Inputs/thinlto_expect1.proftext
+// RUN: %clang -O2 -fexperimental-new-pass-manager -flto=thin -g -fprofile-use=%t1.profdata -c -o %t1.bo %s
+// RUN: llvm-lto -thinlto -o %t %t1.bo
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -emit-obj -Rpass-analysis=info 2>&1 | FileCheck %s -check-prefix=CHECK-REMARK
+// RUN: llvm-profdata merge -o %t2.profdata %S/Inputs/thinlto_expect2.proftext
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj -Wmisexpect 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj 2>&1 | FileCheck %s -allow-empty -check-prefix=CHECK-NOWARNING
+
+int sum;
+__attribute__((noinline)) void bar() {
+  sum = 1234;
+}
+
+__attribute__((noinline)) void foo(int m) {
+  if (__builtin_expect(m > 9, 1))
+bar();
+}
+// CHECK-REMARK: remark: In {{.*}}.c:
+// CHECK-WARNING: warning: Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24) of profiled executions.
+// CHECK-NOWARNING-NOT: warning: {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24)
Index: clang/test/CodeGen/Inputs/thinlto_expect2.proftext
===
--- /dev/null
+++ clang/test/CodeGen/Inputs/thinlto_expect2.proftext
@@ -0,0 +1,20 @@
+# CSIR level Instrumentation Flag
+:csir
+foo
+# Func Hash:
+25571299074
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
+foo
+# Func Hash:
+1152921530178146050
+# Num Counters:
+2
+# Counter Values:
+24
+12
+
Index: clang/test/CodeGen/Inputs/thinlto_expect1.proftext
===
--- /dev/null
+++ clang/test/CodeGen/Inputs/thinlto_expect1.proftext
@@ -0,0 +1,11 @@
+# IR level Instrumentation Flag
+:ir
+foo
+# Func Hash:
+25571299074
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -151,6 +151,25 @@
   FrontendTimesIsEnabled = TimePasses;
   llvm::TimePassesIsEnabled = TimePasses;
 }
+BackendConsumer(BackendAction Action, DiagnosticsEngine &Diags,
+const HeaderSearchOptions &HeaderSearchOpts,
+const PreprocessorOptions &PPOpts,
+const CodeGenOptions &CodeGenOpts,
+const TargetOptions &TargetOpts,
+const LangOptions &LangOpts, bool TimePasses,
+SmallVector LinkModules, LLVMContext &C,
+CoverageSourceInfo *CoverageInfo = nullptr)
+: Diags(Diags), Action(Action), HeaderSearchOpts(HeaderSearchOpts),
+  CodeGenOpts(CodeGenOpts), TargetOpts(TargetOpts), LangOpts(LangOpts),
+  Context(nullptr),
+  LLVMIRGeneration("irgen", "LLVM IR Generation Time"),
+  LLVMIRGenerationRefCount(0),
+  Gen(CreateLLVMCodeGen(Diags, "", HeaderSearchOpts, PPOpts,
+CodeGenOpts, C, CoverageInfo)),
+  LinkModules

[PATCH] D72523: [remark][diagnostics] Using clang diagnostic handler for IR input files

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 237810.
xur added a comment.

Thanks Teresa for the suggestion: using the print() function in the llvm 
diagnosticPrinter is a lot better than duplicating the code. I changed the 
patch to use print() function. Also added comment as suggested by Teresa.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72523/new/

https://reviews.llvm.org/D72523

Files:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/test/CodeGen/Inputs/thinlto_expect1.proftext
  clang/test/CodeGen/Inputs/thinlto_expect2.proftext
  clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
  clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll

Index: clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
===
--- clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
+++ clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
@@ -28,7 +28,7 @@
 ; YAML-NEXT: ...
 
 ; Next try with pass remarks to stderr
-; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -mllvm -pass-remarks=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
+; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -Rpass=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
 
 ; CHECK: tinkywinky inlined into main with (cost=0, threshold=337) (hotness: 300)
 
Index: clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
===
--- /dev/null
+++ clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
@@ -0,0 +1,24 @@
+// Test clang diagnositic handler works in IR file compilaiton.
+
+// REQUIRES: x86-registered-target
+
+// RUN: llvm-profdata merge -o %t1.profdata %S/Inputs/thinlto_expect1.proftext
+// RUN: %clang -O2 -fexperimental-new-pass-manager -flto=thin -g -fprofile-use=%t1.profdata -c -o %t1.bo %s
+// RUN: llvm-lto -thinlto -o %t %t1.bo
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -emit-obj -Rpass-analysis=info 2>&1 | FileCheck %s -check-prefix=CHECK-REMARK
+// RUN: llvm-profdata merge -o %t2.profdata %S/Inputs/thinlto_expect2.proftext
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj -Wmisexpect 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj 2>&1 | FileCheck %s -allow-empty -check-prefix=CHECK-NOWARNING
+
+int sum;
+__attribute__((noinline)) void bar() {
+  sum = 1234;
+}
+
+__attribute__((noinline)) void foo(int m) {
+  if (__builtin_expect(m > 9, 1))
+bar();
+}
+// CHECK-REMARK: remark: {{.*}}.c:
+// CHECK-WARNING: warning: Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24) of profiled executions.
+// CHECK-NOWARNING-NOT: warning: {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24)
Index: clang/test/CodeGen/Inputs/thinlto_expect2.proftext
===
--- /dev/null
+++ clang/test/CodeGen/Inputs/thinlto_expect2.proftext
@@ -0,0 +1,20 @@
+# CSIR level Instrumentation Flag
+:csir
+foo
+# Func Hash:
+25571299074
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
+foo
+# Func Hash:
+1152921530178146050
+# Num Counters:
+2
+# Counter Values:
+24
+12
+
Index: clang/test/CodeGen/Inputs/thinlto_expect1.proftext
===
--- /dev/null
+++ clang/test/CodeGen/Inputs/thinlto_expect1.proftext
@@ -0,0 +1,11 @@
+# IR level Instrumentation Flag
+:ir
+foo
+# Func Hash:
+25571299074
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -151,6 +151,25 @@
   FrontendTimesIsEnabled = TimePasses;
   llvm::TimePassesIsEnabled = TimePasses;
 }
+BackendConsumer(BackendAction Action, DiagnosticsEngine &Diags,
+const HeaderSearchOptions &HeaderSearchOpts,
+const PreprocessorOptions &PPOpts,
+const CodeGenOptions &CodeGenOpts,
+const TargetOptions &TargetOpts,
+const LangOptions &LangOpts, bool TimePasses,
+SmallVector LinkModules, LLVMContext &C,
+CoverageSourceInfo *CoverageInfo = nullptr)
+: Diags(Diags), Action(Action), HeaderSearchOpts(HeaderSearchOpts),
+  CodeGenOpts(CodeGenOpts), TargetOpts(TargetOpts), LangOpts(LangOpts),
+  Context(nullptr),
+  LLVMIRGeneration("irgen", "LLVM IR Generation Time"),
+  LL

[PATCH] D72523: [remark][diagnostics] Using clang diagnostic handler for IR input files

[Rong Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG60d39479221d: [remark][diagnostics] Using clang diagnostic 
handler for IR input files (authored by xur).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D72523?vs=237810&id=238129#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72523/new/

https://reviews.llvm.org/D72523

Files:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/test/CodeGen/Inputs/thinlto_expect1.proftext
  clang/test/CodeGen/Inputs/thinlto_expect2.proftext
  clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
  clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll

Index: clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
===
--- clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
+++ clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll
@@ -28,7 +28,7 @@
 ; YAML-NEXT: ...
 
 ; Next try with pass remarks to stderr
-; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -mllvm -pass-remarks=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
+; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -Rpass=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
 
 ; CHECK: tinkywinky inlined into main with (cost=0, threshold=337) (hotness: 300)
 
Index: clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
===
--- /dev/null
+++ clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c
@@ -0,0 +1,24 @@
+// Test clang diagnositic handler works in IR file compilation.
+
+// REQUIRES: x86-registered-target
+
+// RUN: llvm-profdata merge -o %t1.profdata %S/Inputs/thinlto_expect1.proftext
+// RUN: %clang -O2 -fexperimental-new-pass-manager -flto=thin -g -fprofile-use=%t1.profdata -c -o %t1.bo %s
+// RUN: llvm-lto -thinlto -o %t %t1.bo
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -emit-obj -Rpass-analysis=info 2>&1 | FileCheck %s -check-prefix=CHECK-REMARK
+// RUN: llvm-profdata merge -o %t2.profdata %S/Inputs/thinlto_expect2.proftext
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj -Wmisexpect 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj 2>&1 | FileCheck %s -allow-empty -check-prefix=CHECK-NOWARNING
+
+int sum;
+__attribute__((noinline)) void bar() {
+  sum = 1234;
+}
+
+__attribute__((noinline)) void foo(int m) {
+  if (__builtin_expect(m > 9, 1))
+bar();
+}
+// CHECK-REMARK: remark: {{.*}}.c:
+// CHECK-WARNING: warning: Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24) of profiled executions.
+// CHECK-NOWARNING-NOT: warning: {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24)
Index: clang/test/CodeGen/Inputs/thinlto_expect2.proftext
===
--- /dev/null
+++ clang/test/CodeGen/Inputs/thinlto_expect2.proftext
@@ -0,0 +1,20 @@
+# CSIR level Instrumentation Flag
+:csir
+foo
+# Func Hash:
+25571299074
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
+foo
+# Func Hash:
+1152921530178146050
+# Num Counters:
+2
+# Counter Values:
+24
+12
+
Index: clang/test/CodeGen/Inputs/thinlto_expect1.proftext
===
--- /dev/null
+++ clang/test/CodeGen/Inputs/thinlto_expect1.proftext
@@ -0,0 +1,11 @@
+# IR level Instrumentation Flag
+:ir
+foo
+# Func Hash:
+25571299074
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -151,6 +151,29 @@
   FrontendTimesIsEnabled = TimePasses;
   llvm::TimePassesIsEnabled = TimePasses;
 }
+
+// This constructor is used in installing an empty BackendConsumer
+// to use the clang diagnostic handler for IR input files. It avoids
+// initializing the OS field.
+BackendConsumer(BackendAction Action, DiagnosticsEngine &Diags,
+const HeaderSearchOptions &HeaderSearchOpts,
+const PreprocessorOptions &PPOpts,
+const CodeGenOptions &CodeGenOpts,
+const TargetOptions &TargetOpts,
+const LangOptions &LangOpts, bool TimePasses,
+SmallVector LinkModules, LLVMContext &C,
+CoverageSourceInfo *CoverageInfo = nullptr)
+: Diags(D

[PATCH] D91673: [PGO] Enable preinline and cleanup when optimize for size

[Rong Xu via Phabricator via cfe-commits]

xur accepted this revision.
xur added a comment.

Sorry to the late response (I somehow missed the reply).

(1) Can we add some comments to explain the reason for enabling this for -Oz -- 
I think the comments from @rnk is useful to some people (at least to me).

(2) Also please add a testcase.

Feel free to commit to address the above two issues.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91673/new/

https://reviews.llvm.org/D91673

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91813: [PGO] verify BFI counts after loading profile data

[Rong Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG54e03d03a7a4: [PGO] Verify BFI counts after loading profile 
data (authored by xur).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D91813?vs=306763&id=311740#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91813/new/

https://reviews.llvm.org/D91813

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/test/Transforms/PGOProfile/Inputs/bfi_verification.proftext
  llvm/test/Transforms/PGOProfile/bfi_verification.ll

Index: llvm/test/Transforms/PGOProfile/bfi_verification.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/bfi_verification.ll
@@ -0,0 +1,111 @@
+; Note: Verify bfi counter after loading the profile.
+; RUN: llvm-profdata merge %S/Inputs/bfi_verification.proftext -o %t.profdata
+; RUN: opt < %s -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-verify-bfi-ratio=2 -pgo-verify-bfi=true -pass-remarks-analysis=pgo 2>&1 | FileCheck %s --check-prefix=THRESHOLD-CHECK
+; RUN: opt < %s -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-verify-hot-bfi=true -pass-remarks-analysis=pgo 2>&1 | FileCheck %s --check-prefix=HOTONLY-CHECK
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.basket = type { %struct.arc*, i64, i64 }
+%struct.arc = type { i64, %struct.node*, %struct.node*, i32, %struct.arc*, %struct.arc*, i64, i64 }
+%struct.node = type { i64, i32, %struct.node*, %struct.node*, %struct.node*, %struct.node*, %struct.arc*, %struct.arc*, %struct.arc*, %struct.arc*, i64, i64, i32, i32 }
+
+@perm = internal unnamed_addr global [351 x %struct.basket*] zeroinitializer, align 16
+
+define dso_local void @sort_basket(i64 %min, i64 %max) {
+entry:
+  %add = add nsw i64 %min, %max
+  %div = sdiv i64 %add, 2
+  %arrayidx = getelementptr inbounds [351 x %struct.basket*], [351 x %struct.basket*]* @perm, i64 0, i64 %div
+  %0 = load %struct.basket*, %struct.basket** %arrayidx, align 8
+  %abs_cost = getelementptr inbounds %struct.basket, %struct.basket* %0, i64 0, i32 2
+  %1 = load i64, i64* %abs_cost, align 8
+  br label %do.body
+
+do.body:
+  %r.0 = phi i64 [ %max, %entry ], [ %r.2, %if.end ]
+  %l.0 = phi i64 [ %min, %entry ], [ %l.2, %if.end ]
+  br label %while.cond
+
+while.cond:
+  %l.1 = phi i64 [ %l.0, %do.body ], [ %inc, %while.body ]
+  %arrayidx1 = getelementptr inbounds [351 x %struct.basket*], [351 x %struct.basket*]* @perm, i64 0, i64 %l.1
+  %2 = load %struct.basket*, %struct.basket** %arrayidx1, align 8
+  %abs_cost2 = getelementptr inbounds %struct.basket, %struct.basket* %2, i64 0, i32 2
+  %3 = load i64, i64* %abs_cost2, align 8
+  %cmp = icmp sgt i64 %3, %1
+  br i1 %cmp, label %while.body, label %while.cond3
+
+while.body:
+  %inc = add nsw i64 %l.1, 1
+  br label %while.cond
+
+while.cond3:
+  %r.1 = phi i64 [ %r.0, %while.cond ], [ %dec, %while.body7 ]
+  %arrayidx4 = getelementptr inbounds [351 x %struct.basket*], [351 x %struct.basket*]* @perm, i64 0, i64 %r.1
+  %4 = load %struct.basket*, %struct.basket** %arrayidx4, align 8
+  %abs_cost5 = getelementptr inbounds %struct.basket, %struct.basket* %4, i64 0, i32 2
+  %5 = load i64, i64* %abs_cost5, align 8
+  %cmp6 = icmp sgt i64 %1, %5
+  br i1 %cmp6, label %while.body7, label %while.end8
+
+while.body7:
+  %dec = add nsw i64 %r.1, -1
+  br label %while.cond3
+
+while.end8:
+  %cmp9 = icmp slt i64 %l.1, %r.1
+  br i1 %cmp9, label %if.then, label %if.end
+
+if.then:
+  %6 = bitcast %struct.basket** %arrayidx1 to i64*
+  %7 = load i64, i64* %6, align 8
+  store %struct.basket* %4, %struct.basket** %arrayidx1, align 8
+  %8 = bitcast %struct.basket** %arrayidx4 to i64*
+  store i64 %7, i64* %8, align 8
+  br label %if.end
+
+if.end:
+  %cmp14 = icmp sgt i64 %l.1, %r.1
+  %not.cmp14 = xor i1 %cmp14, true
+  %9 = zext i1 %not.cmp14 to i64
+  %r.2 = sub i64 %r.1, %9
+  %not.cmp1457 = xor i1 %cmp14, true
+  %inc16 = zext i1 %not.cmp1457 to i64
+  %l.2 = add nsw i64 %l.1, %inc16
+  %cmp19 = icmp sgt i64 %l.2, %r.2
+  br i1 %cmp19, label %do.end, label %do.body
+
+do.end:
+  %cmp20 = icmp sgt i64 %r.2, %min
+  br i1 %cmp20, label %if.then21, label %if.end22
+
+if.then21:
+  call void @sort_basket(i64 %min, i64 %r.2)
+  br label %if.end22
+
+if.end22:
+  %cmp23 = icmp slt i64 %l.2, %max
+  %cmp24 = icmp slt i64 %l.2, 51
+  %or.cond = and i1 %cmp23, %cmp24
+  br i1 %or.cond, label %if.then25, label %if.end26
+
+if.then25:
+  call void @sort_basket(i64 %l.2, i64 %max)
+  br label %if.end26
+
+if.end26:
+  ret void
+}
+; THRESHOLD-CHECK: remark: :0:0: BB do.body Count=39637749 BFI_Count=40801304
+; THRESHOLD-CHECK: remark: :0:0: BB while.cond Count=80655628 BFI_Count=83956530
+; THRESHOLD-CHECK: re

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

This breaks  llvm/test/Bitcode/attributes.ll.
The added test code was obviously wrong.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90275/new/

https://reviews.llvm.org/D90275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92493: [IR] Add hot to function attributes and use hot/cold attribute in function section prefix/suffix

[Rong Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3733463dbb58: [IR][PGO] Add hot func attribute and use 
hot/cold attribute in func section (authored by xur).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D92493?vs=312273&id=312664#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92493/new/

https://reviews.llvm.org/D92493

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/attributes.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/CodeGen/CodeGenPrepare.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/test/Bitcode/attributes.ll
  llvm/test/CodeGen/X86/hot-unlikely-section-prefix.ll
  llvm/test/MC/AsmParser/function_hot_attr.ll

Index: llvm/test/MC/AsmParser/function_hot_attr.ll
===
--- /dev/null
+++ llvm/test/MC/AsmParser/function_hot_attr.ll
@@ -0,0 +1,13 @@
+; Test hot function attribute
+; RUN: llc < %s | FileCheck %s
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: hot noinline norecurse nounwind readnone uwtable
+define dso_local i32 @hot4() #0 {
+entry:
+  ret i32 1
+}
+; CHECK: .section.text.hot.,"ax",@progbits
+; CHECK: .globl  hot4
+
+attributes #0 = { hot noinline norecurse nounwind readnone uwtable }
Index: llvm/test/CodeGen/X86/hot-unlikely-section-prefix.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/hot-unlikely-section-prefix.ll
@@ -0,0 +1,101 @@
+; Test hot or unlikely section postfix based on profile and user annotation.
+; RUN: llc < %s | FileCheck %s
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: inlinehint norecurse nounwind readnone uwtable
+define dso_local i32 @hot1() #0 !prof !31 {
+entry:
+  ret i32 1
+}
+; CHECK: .section.text.hot.,"ax",@progbits
+; CHECK: .globl  hot1
+
+; Function Attrs: cold norecurse nounwind readnone uwtable
+define dso_local i32 @cold1() #1 !prof !32 {
+entry:
+  ret i32 1
+}
+; CHECK: .section.text.unlikely.,"ax",@progbits
+; CHECK: .globl  cold1
+
+; Function Attrs: cold inlinehint noinline norecurse nounwind optsize readnone uwtable
+define dso_local i32 @hot2() #2 !prof !31 {
+entry:
+  ret i32 1
+}
+; CHECK: .section.text.hot.,"ax",@progbits
+; CHECK: .globl  hot2
+
+define dso_local i32 @normal() {
+entry:
+  ret i32 1
+}
+; CHECK: text
+; CHECK: .globl  normal
+
+; Function Attrs: hot noinline norecurse nounwind readnone uwtable
+define dso_local i32 @hot3() #3 !prof !32 {
+entry:
+  ret i32 1
+}
+; CHECK: .section.text.hot.,"ax",@progbits
+; CHECK: .globl  hot3
+
+; Function Attrs: cold noinline norecurse nounwind optsize readnone uwtable
+define dso_local i32 @cold2() #4 {
+entry:
+  ret i32 1
+}
+; CHECK: .section.text.unlikely.,"ax",@progbits
+; CHECK: .globl  cold2
+
+; Function Attrs: hot noinline norecurse nounwind readnone uwtable
+define dso_local i32 @hot4() #3 {
+entry:
+  ret i32 1
+}
+; CHECK: .section.text.hot.,"ax",@progbits
+; CHECK: .globl  hot4
+
+attributes #0 = { inlinehint norecurse nounwind readnone uwtable }
+attributes #1 = { cold norecurse nounwind readnone uwtable }
+attributes #2 = { cold inlinehint noinline norecurse nounwind optsize readnone uwtable }
+attributes #3 = { hot noinline norecurse nounwind readnone uwtable }
+attributes #4 = { cold noinline norecurse nounwind optsize readnone uwtable }
+
+!llvm.module.flags = !{!0, !1}
+!llvm.ident = !{!30}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{i32 1, !"ProfileSummary", !2}
+!2 = !{!3, !4, !5, !6, !7, !8, !9, !10, !11, !12}
+!3 = !{!"ProfileFormat", !"InstrProf"}
+!4 = !{!"TotalCount", i64 402020}
+!5 = !{!"MaxCount", i64 20}
+!6 = !{!"MaxInternalCount", i64 2000}
+!7 = !{!"MaxFunctionCount", i64 20}
+!8 = !{!"NumCounts", i64 7}
+!9 = !{!"NumFunctions", i64 5}
+!10 = !{!"IsPartialProfile", i64 0}
+!11 = !{!"PartialProfileRatio", double 0.00e+00}
+!12 = !{!"DetailedSummary", !13}
+!13 = !{!14, !15, !16, !17, !18, !19, !20, !21, !22, !23, !24, !25, !26, !27, !28, !29}
+!14 = !{i32 1, i64 20, i32 1}
+!15 = !{i32 10, i64 20, i32 1}
+!16 = !{i32 20, i64 20, i32 1}
+!17 = !{i32 30, i64 20, i32 1}
+!18 = !{i32 40, i64 20, i32 1}
+!19 = !{i32 50, i64 10, i32 3}
+!20 = !{i32 60, i64 10, i32 3}
+!21 = !{i32 70, i64 10, i32 3}
+!22 = !{i32 80, i64 10, i32 3}
+!23 = !{i32 90, i64 10, i32 3}
+!24 = !{i32 95, i64 10, i32 3}
+!25 = !{i32 99, i64 10, i32 3}
+!26 = !{i32 999000, i64 2000

[PATCH] D91673: [PGO] Enable preinline and cleanup when optimize for size

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

This is probably OK for -Os (SizeLevel == 1), but we need to be careful with Oz 
(SizeLevel == 2).
We already know that enabling preinliner in general will reduce size -- as the 
preinliner is pretty conservative. But there will be cases size will be 
increased.

I would like more test results (like bootstrap clang) before committing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91673/new/

https://reviews.llvm.org/D91673

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96487: Restore diagnostic handler after CodeGenAction::ExecuteAction

[Rong Xu via Phabricator via cfe-commits]

xur accepted this revision.
xur added a comment.
This revision is now accepted and ready to land.

This looks fine to me. Thanks for fixing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96487/new/

https://reviews.llvm.org/D96487

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

[Rong Xu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5fdaaf7fd8f3: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) 
profile loader (authored by xur).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D107878?vs=367321&id=367381#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107878/new/

https://reviews.llvm.org/D107878

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/CodeGen/MIRSampleProfile.h
  llvm/include/llvm/CodeGen/MachineDominators.h
  llvm/include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
  llvm/lib/CodeGen/CMakeLists.txt
  llvm/lib/CodeGen/MIRSampleProfile.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/test/CodeGen/X86/Inputs/fsloader.afdo
  llvm/test/CodeGen/X86/fsafdo_test2.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -284,6 +284,9 @@
   P->CSAction = PGOOptions::CSIRUse;
 }
   }
+  if (TM)
+TM->setPGOOption(P);
+
   LoopAnalysisManager LAM;
   FunctionAnalysisManager FAM;
   CGSCCAnalysisManager CGAM;
Index: llvm/test/CodeGen/X86/fsafdo_test2.ll
===
--- llvm/test/CodeGen/X86/fsafdo_test2.ll
+++ llvm/test/CodeGen/X86/fsafdo_test2.ll
@@ -1,4 +1,7 @@
 ; RUN: llc -enable-fs-discriminator < %s | FileCheck %s
+; RUN: llvm-profdata merge --sample -profile-isfs -o %t.afdo %S/Inputs/fsloader.afdo
+; RUN: llc -enable-fs-discriminator -fs-profile-file=%t.afdo -show-fs-branchprob -disable-ra-fsprofile-loader=false -disable-layout-fsprofile-loader=false < %s 2>&1 | FileCheck %s --check-prefix=LOADER
+;
 ;;
 ;; C source code for the test (compiler at -O3):
 ;; // A test case for loop unroll.
@@ -50,6 +53,25 @@
 ; CHECK: .byte   1
 ; CHECK: .size   __llvm_fs_discriminator__, 1
 
+;; Check that new branch probs are generated.
+; LOADER: Set branch fs prob: MBB (1 -> 3): unroll.c:22:11-->unroll.c:24:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x7aca7894 / 0x8000 = 95.93%
+; LOADER: Set branch fs prob: MBB (1 -> 2): unroll.c:22:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x0535876c / 0x8000 = 4.07%
+; LOADER: Set branch fs prob: MBB (3 -> 5): unroll.c:24:11-->unroll.c:22:11 W=283590  0x3000 / 0x8000 = 37.50% --> 0x7aca7894 / 0x8000 = 95.93%
+; LOADER: Set branch fs prob: MBB (3 -> 4): unroll.c:24:11 W=283590  0x5000 / 0x8000 = 62.50% --> 0x0535876c / 0x8000 = 4.07%
+; LOADER: Set branch fs prob: MBB (5 -> 8): unroll.c:22:11-->unroll.c:24:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x021c112e / 0x8000 = 1.65%
+; LOADER: Set branch fs prob: MBB (5 -> 7): unroll.c:22:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x7de3eed2 / 0x8000 = 98.35%
+; LOADER: Set branch fs prob: MBB (8 -> 10): unroll.c:24:11-->unroll.c:22:11 W=283590  0x3000 / 0x8000 = 37.50% --> 0x / 0x8000 = 0.00%
+; LOADER: Set branch fs prob: MBB (8 -> 9): unroll.c:24:11 W=283590  0x5000 / 0x8000 = 62.50% --> 0x8000 / 0x8000 = 100.00%
+; LOADER: Set branch fs prob: MBB (10 -> 12): unroll.c:22:11-->unroll.c:24:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x7aca7894 / 0x8000 = 95.93%
+; LOADER: Set branch fs prob: MBB (10 -> 11): unroll.c:22:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x0535876c / 0x8000 = 4.07%
+; LOADER: Set branch fs prob: MBB (12 -> 14): unroll.c:24:11-->unroll.c:22:11 W=283590  0x3000 / 0x8000 = 37.50% --> 0x02012507 / 0x8000 = 1.57%
+; LOADER: Set branch fs prob: MBB (12 -> 13): unroll.c:24:11 W=283590  0x5000 / 0x8000 = 62.50% --> 0x7dfedaf9 / 0x8000 = 98.43%
+; LOADER: Set branch fs prob: MBB (14 -> 16): unroll.c:22:11-->unroll.c:24:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x0a5856e1 / 0x8000 = 8.08%
+; LOADER: Set branch fs prob: MBB (14 -> 15): unroll.c:22:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x75a7a91f / 0x8000 = 91.92%
+; LOADER: Set branch fs prob: MBB (16 -> 18): unroll.c:24:11-->unroll.c:19:3 W=283590  0x3000 / 0x8000 = 37.50% --> 0x16588166 / 0x8000 = 17.46%
+; LOADER: Set branch fs prob: MBB (16 -> 17): unroll.c:24:11 W=283590  0x5000 / 0x8000 = 62.50% --> 0x69a77e9a / 0x8000 = 82.54%
+
+
 target triple = "x86_64-unknown-linux-gnu"
 
 @sum = dso_local local_unnamed_addr

[PATCH] D136397: [Clang] Change AnonStructIds in MangleContext to per-function based

[Rong Xu via Phabricator via cfe-commits]

xur created this revision.
xur added reviewers: rsmith, davidxl.
Herald added a subscriber: wenlei.
Herald added a project: All.
xur requested review of this revision.

Clang is generating different mangled names for the same lambda function in 
build that are
slightly different (like from non-related source/Macro change). This is due to 
the fact that clang uses a
cross-translation-unit sequential string "$_" in lambda's mangled name. 
Here, "n" is the
AnonStructIds field in MangleContext.

Different mangled names for unchanged function is undesirable: it makes perf 
comparison harder,
and can cause some unnecessary profile mismatch in SampleFDO.

This patch changes AnonStructIds to a per-function based seq number if the
DeclContext is a function.

I hold the change for Microsoft mangling and only change Itanium mangling.


https://reviews.llvm.org/D136397

Files:
  clang/include/clang/AST/Mangle.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/test/CodeGen/attr-function-return.cpp
  clang/test/CodeGenCXX/catch-undef-behavior.cpp
  clang/test/CodeGenCXX/cxx1y-init-captures-eh.cpp
  clang/test/CodeGenCXX/cxx1y-init-captures.cpp
  clang/test/CodeGenCXX/lambda-expressions-nested-linkage.cpp
  clang/test/CodeGenCXX/lambda-expressions.cpp
  clang/test/CodeGenCXX/mangle-lambdas.cpp
  clang/test/CodeGenCXX/nrvo.cpp
  clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  clang/test/CodeGenObjCXX/block-nested-in-lambda.mm
  clang/test/CodeGenObjCXX/lambda-expressions.mm
  clang/test/CodeGenObjCXX/property-lvalue-lambda.mm

Index: clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
===
--- clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
+++ clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
@@ -36,12 +36,12 @@
   // [x setBlk: operator+([x blk], [] {})]
 
   // CHECK: call void{{.*}}@objc_msgSend{{.*}}
-  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_2EU13block_pointerFvvES4_T_"
+  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_0EU13block_pointerFvvES4_T_"
   // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} [[PLUS]])
   x.blk += [] {};
 
   // CHECK: call void{{.*}}@objc_msgSend{{.*}}
-  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_3EPFvvES4_T_"
+  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_1EPFvvES4_T_"
   // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} [[PLUS]])
   x.fnptr += [] {};
 }
Index: clang/test/CodeGenObjCXX/lambda-expressions.mm
===
--- clang/test/CodeGenObjCXX/lambda-expressions.mm
+++ clang/test/CodeGenObjCXX/lambda-expressions.mm
@@ -38,7 +38,7 @@
 // ARC: call i8* @llvm.objc.retainBlock
 // ARC: call void @llvm.objc.release
 // ARC-LABEL: define internal noundef i32 @___Z2f2v_block_invoke
-// ARC: call noundef i32 @"_ZZ2f2vENK3$_1clEv
+// ARC: call noundef i32 @"_ZZ2f2vENK3$_0clEv
 
 template  void take_lambda(T &&lambda) { lambda(); }
 void take_block(void (^block)()) { block(); }
@@ -66,7 +66,7 @@
 // ARC:   %[[CAPTURE0:.*]] = getelementptr inbounds %[[LAMBDACLASS]], %[[LAMBDACLASS]]* %{{.*}}, i32 0, i32 0
 // ARC:   store i32 %{{.*}}, i32* %[[CAPTURE0]]
 
-// ARC: define internal void @"_ZZN13LambdaCapture4foo1ERiENK3$_3clEv"(%[[LAMBDACLASS]]* {{[^,]*}} %{{.*}})
+// ARC: define internal void @"_ZZN13LambdaCapture4foo1ERiENK3$_0clEv"(%[[LAMBDACLASS]]* {{[^,]*}} %{{.*}})
 // ARC:   %[[BLOCK:.*]] = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>
 // ARC:   %[[CAPTURE1:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>, <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>* %[[BLOCK]], i32 0, i32 5
 // ARC:   store i32 %{{.*}}, i32* %[[CAPTURE1]]
@@ -75,11 +75,11 @@
 // ARC-NOT: @llvm.objc.storeStrong(
 // ARC: ret void
 
-// ARC: define internal void @"___ZZN13LambdaCapture4foo1ERiENK3$_3clEv_block_invoke"
+// ARC: define internal void @"___ZZN13LambdaCapture4foo1ERiENK3$_0clEv_block_invoke"
 // ARC:   %[[CAPTURE2:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>, <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>* %{{.*}}, i32 0, i32 5
 // ARC:   store i32 %{{.*}}, i32* %[[CAPTURE2]]
 
-// ARC: define internal void @"___ZZN13LambdaCapture4foo1ERiENK3$_3clEv_block_invoke_2"(i8* noundef %{{.*}})
+// ARC: define internal void @"___ZZN13LambdaCapture4foo1ERiENK3$_0clEv_block_invoke_2"(i8* noundef %{{.*}})
 // ARC:   %[[CAPTURE3:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>, <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>* %{{.*}}, i32 0, i32 5
 // ARC:   %[[V1:.*]] = load i32, i32* %[[CAPTURE3]]
 // ARC:   store i32 %[[V1]], i32* @_ZN13LambdaCapture1iE
@@ -141,11 +141,11 @@
 // Check that the delegating invoke function doesn't destruct the Weak object
 // that is passed.
 
-// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvEN3$_58__invokeENS_4WeakE"(
-

[PATCH] D136397: [Clang] Change AnonStructIds in MangleContext to per-function based

[Rong Xu via Phabricator via cfe-commits]

xur marked an inline comment as done.
xur added inline comments.



Comment at: clang/include/clang/AST/Mangle.h:97
+
+// If FunctionDecl is passed in, the anonnousstructID will be per-function
+// based.

rsmith wrote:
> 
Fixed. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136397/new/

https://reviews.llvm.org/D136397

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136397: [Clang] Change AnonStructIds in MangleContext to per-function based

[Rong Xu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
xur marked an inline comment as done.
Closed by commit rG6cee5393371f: [Clang] Change AnonStructIds in MangleContext 
to per-function based (authored by xur).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D136397?vs=469415&id=470052#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136397/new/

https://reviews.llvm.org/D136397

Files:
  clang/include/clang/AST/Mangle.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/test/CodeGen/attr-function-return.cpp
  clang/test/CodeGenCXX/catch-undef-behavior.cpp
  clang/test/CodeGenCXX/cxx1y-init-captures-eh.cpp
  clang/test/CodeGenCXX/cxx1y-init-captures.cpp
  clang/test/CodeGenCXX/lambda-expressions-nested-linkage.cpp
  clang/test/CodeGenCXX/lambda-expressions.cpp
  clang/test/CodeGenCXX/mangle-lambdas.cpp
  clang/test/CodeGenCXX/nrvo.cpp
  clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  clang/test/CodeGenObjCXX/block-nested-in-lambda.mm
  clang/test/CodeGenObjCXX/lambda-expressions.mm
  clang/test/CodeGenObjCXX/property-lvalue-lambda.mm

Index: clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
===
--- clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
+++ clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
@@ -36,12 +36,12 @@
   // [x setBlk: operator+([x blk], [] {})]
 
   // CHECK: call void{{.*}}@objc_msgSend{{.*}}
-  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_2EU13block_pointerFvvES4_T_"
+  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_0EU13block_pointerFvvES4_T_"
   // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} [[PLUS]])
   x.blk += [] {};
 
   // CHECK: call void{{.*}}@objc_msgSend{{.*}}
-  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_3EPFvvES4_T_"
+  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_1EPFvvES4_T_"
   // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} [[PLUS]])
   x.fnptr += [] {};
 }
Index: clang/test/CodeGenObjCXX/lambda-expressions.mm
===
--- clang/test/CodeGenObjCXX/lambda-expressions.mm
+++ clang/test/CodeGenObjCXX/lambda-expressions.mm
@@ -38,7 +38,7 @@
 // ARC: call i8* @llvm.objc.retainBlock
 // ARC: call void @llvm.objc.release
 // ARC-LABEL: define internal noundef i32 @___Z2f2v_block_invoke
-// ARC: call noundef i32 @"_ZZ2f2vENK3$_1clEv
+// ARC: call noundef i32 @"_ZZ2f2vENK3$_0clEv
 
 template  void take_lambda(T &&lambda) { lambda(); }
 void take_block(void (^block)()) { block(); }
@@ -66,7 +66,7 @@
 // ARC:   %[[CAPTURE0:.*]] = getelementptr inbounds %[[LAMBDACLASS]], %[[LAMBDACLASS]]* %{{.*}}, i32 0, i32 0
 // ARC:   store i32 %{{.*}}, i32* %[[CAPTURE0]]
 
-// ARC: define internal void @"_ZZN13LambdaCapture4foo1ERiENK3$_3clEv"(%[[LAMBDACLASS]]* {{[^,]*}} %{{.*}})
+// ARC: define internal void @"_ZZN13LambdaCapture4foo1ERiENK3$_0clEv"(%[[LAMBDACLASS]]* {{[^,]*}} %{{.*}})
 // ARC:   %[[BLOCK:.*]] = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>
 // ARC:   %[[CAPTURE1:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>, <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>* %[[BLOCK]], i32 0, i32 5
 // ARC:   store i32 %{{.*}}, i32* %[[CAPTURE1]]
@@ -75,11 +75,11 @@
 // ARC-NOT: @llvm.objc.storeStrong(
 // ARC: ret void
 
-// ARC: define internal void @"___ZZN13LambdaCapture4foo1ERiENK3$_3clEv_block_invoke"
+// ARC: define internal void @"___ZZN13LambdaCapture4foo1ERiENK3$_0clEv_block_invoke"
 // ARC:   %[[CAPTURE2:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>, <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>* %{{.*}}, i32 0, i32 5
 // ARC:   store i32 %{{.*}}, i32* %[[CAPTURE2]]
 
-// ARC: define internal void @"___ZZN13LambdaCapture4foo1ERiENK3$_3clEv_block_invoke_2"(i8* noundef %{{.*}})
+// ARC: define internal void @"___ZZN13LambdaCapture4foo1ERiENK3$_0clEv_block_invoke_2"(i8* noundef %{{.*}})
 // ARC:   %[[CAPTURE3:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>, <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i32 }>* %{{.*}}, i32 0, i32 5
 // ARC:   %[[V1:.*]] = load i32, i32* %[[CAPTURE3]]
 // ARC:   store i32 %[[V1]], i32* @_ZN13LambdaCapture1iE
@@ -141,11 +141,11 @@
 // Check that the delegating invoke function doesn't destruct the Weak object
 // that is passed.
 
-// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvEN3$_58__invokeENS_4WeakE"(
-// ARC: call void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvEN3$_08__invokeENS_4WeakE"(
+// ARC: call void @"_ZZN14LambdaDelegate4testEvENK3$_0clENS_4WeakE"(
 // ARC-NEXT: ret void
 
-// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvENK

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

[Rong Xu via Phabricator via cfe-commits]

xur created this revision.
xur added reviewers: davidxl, tmsriram.
Herald added a subscriber: wenlei.
Herald added a project: All.
xur requested review of this revision.
Herald added a project: LLVM.

This patch has the following changes:
(1) Handling of internal linkage functions (static functions)
Static functions in FDO have a prefix of source file name, while they do not
have one in SampleFDO. Current implementation does not handle this and we are
not updating the profile for static functions. This patch fixes this.

(2) Handling of -funique-internal-linakge-symbols
Again this is for the internal linkage functions. Option
-funique-internal-linakge-symbols can now be applied to both FDO and SampleFDO
compilation. When it is used, it demangles internal linkage function names and
adds a hash value as the postfix.

When both SampleFDO and FDO profiles use this option, or both
not use this option, changes in (1) should handle this.

Here we also handle when the SampleFDO profile using this option while FDO
profile not using this option, or vice versa.

There is one case where this patch won't work: If one of the profiles used
mangled name and the other does not. For example, if the SampleFDO profile
uses clang c-compiler and without -funique-internal-linakge-symbols, while
the FDO profile uses -funique-internal-linakge-symbols. The SampleFDO profile
contains unmangled names while the FDO profile contains mangled names. If
both profiles use c++ compiler, this won't happen. We think this use case
is rare and does not justify the effort to fix.


https://reviews.llvm.org/D132600

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/ProfileData/SampleProf.h
  llvm/test/tools/llvm-profdata/Inputs/FUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/FUnique.proftext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.proftext
  llvm/test/tools/llvm-profdata/suppl-instr-with-sample-static-func.test
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/MD5.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ThreadPool.h"
@@ -478,7 +479,7 @@
   ZeroCounterRatio = (float)ZeroCntNum / CntNum;
 }
 
-/// Either set all the counters in the instr profile entry \p IFE to -1
+// Either set all the counters in the instr profile entry \p IFE to -1
 /// in order to drop the profile or scale up the counters in \p IFP to
 /// be above hot threshold. We use the ratio of zero counters in the
 /// profile of a function to decide the profile is helpful or harmful
@@ -539,7 +540,73 @@
unsigned InstrProfColdThreshold) {
   // Function to its entry in instr profile.
   StringMap InstrProfileMap;
+  StringMap StaticFuncMap;
   InstrProfSummaryBuilder IPBuilder(ProfileSummaryBuilder::DefaultCutoffs);
+
+  auto checkSampleProfileHasFUnique = [&Reader]() {
+for (const auto &PD : Reader->getProfiles()) {
+  auto &FContext = PD.first;
+  if (FContext.toString().find(FunctionSamples::UniqSuffix) !=
+  std::string::npos) {
+return true;
+  }
+}
+return false;
+  };
+
+  bool SampleProfileHasFUnique = checkSampleProfileHasFUnique();
+
+  auto buildStaticFuncMap = [&StaticFuncMap,
+ SampleProfileHasFUnique](const StringRef &Name) {
+std::string Prefixes[] = {".cpp:", "cc:", ".c:", ".hpp:", ".h:"};
+size_t PrefixPos = StringRef::npos;
+for (auto &Prefix : Prefixes) {
+  PrefixPos = Name.find_insensitive(Prefix);
+  if (PrefixPos == StringRef::npos)
+continue;
+  PrefixPos += Prefix.size();
+  break;
+}
+
+if (PrefixPos == StringRef::npos) {
+  return;
+}
+
+StringRef NewName = Name.drop_front(PrefixPos);
+StringRef FName = Name.substr(0, PrefixPos - 1);
+if (NewName.size() == 0) {
+  return;
+}
+
+// This name should have a static linkage.
+size_t PostfixPos = NewName.find(FunctionSamples::UniqSuffix);
+bool ProfileHasFUnique = (PostfixPos != StringRef::npos);
+
+if (SampleProfileHasFUnique) {
+  // If profile also uses funqiue, nothing to do here.
+  // Otherwise, we need to build the map.
+  if (!ProfileHasFUnique) {
+std::string NStr =
+NewName.str() + getUniqueInternalLinkagePostfix(FName);
+NewName = StringRef(NStr);
+StaticFuncMap[NewName] = Name;
+return;
+  }
+} else {
+  // If profile does not use -funique-internal-linakge-symbols, nothing to
+  // do here. Otherwise, we need to trim.
+  if (ProfileHasFUnique) {
+Ne

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

[Rong Xu via Phabricator via cfe-commits]

xur added inline comments.



Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:606
+} else {
+  StaticFuncMap[NewName] = "---";
+}

davidxl wrote:
> define a macro for the marker string.
Will do.



Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:655
 const sampleprof::FunctionSamples &FS = PD.second;
 auto It = InstrProfileMap.find(FContext.toString());
+if (It == InstrProfileMap.end()) {

davidxl wrote:
> Skip to the next (using continue) when FS is not interesting (cold etc)
OK. 



Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:667
+}
+if (FS.getMaxCountInside() > ColdSampleThreshold &&
 It != InstrProfileMap.end() &&

davidxl wrote:
> continue when Itr == end or when Itr is not cold
Will change.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132600/new/

https://reviews.llvm.org/D132600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 455640.
xur added a comment.

Integrated David's review commends.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132600/new/

https://reviews.llvm.org/D132600

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/ProfileData/SampleProf.h
  llvm/test/tools/llvm-profdata/Inputs/FUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/FUnique.proftext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.proftext
  llvm/test/tools/llvm-profdata/suppl-instr-with-sample-static-func.test
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/MD5.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ThreadPool.h"
@@ -42,6 +43,10 @@
 
 using namespace llvm;
 
+// We use this string to indicate that there are
+// multiple static functions map to the same name.
+const std::string DuplicateNameStr = "";
+
 enum ProfileFormat {
   PF_None = 0,
   PF_Text,
@@ -478,7 +483,7 @@
   ZeroCounterRatio = (float)ZeroCntNum / CntNum;
 }
 
-/// Either set all the counters in the instr profile entry \p IFE to -1
+// Either set all the counters in the instr profile entry \p IFE to -1
 /// in order to drop the profile or scale up the counters in \p IFP to
 /// be above hot threshold. We use the ratio of zero counters in the
 /// profile of a function to decide the profile is helpful or harmful
@@ -539,7 +544,73 @@
unsigned InstrProfColdThreshold) {
   // Function to its entry in instr profile.
   StringMap InstrProfileMap;
+  StringMap StaticFuncMap;
   InstrProfSummaryBuilder IPBuilder(ProfileSummaryBuilder::DefaultCutoffs);
+
+  auto checkSampleProfileHasFUnique = [&Reader]() {
+for (const auto &PD : Reader->getProfiles()) {
+  auto &FContext = PD.first;
+  if (FContext.toString().find(FunctionSamples::UniqSuffix) !=
+  std::string::npos) {
+return true;
+  }
+}
+return false;
+  };
+
+  bool SampleProfileHasFUnique = checkSampleProfileHasFUnique();
+
+  auto buildStaticFuncMap = [&StaticFuncMap,
+ SampleProfileHasFUnique](const StringRef &Name) {
+std::string Prefixes[] = {".cpp:", "cc:", ".c:", ".hpp:", ".h:"};
+size_t PrefixPos = StringRef::npos;
+for (auto &Prefix : Prefixes) {
+  PrefixPos = Name.find_insensitive(Prefix);
+  if (PrefixPos == StringRef::npos)
+continue;
+  PrefixPos += Prefix.size();
+  break;
+}
+
+if (PrefixPos == StringRef::npos) {
+  return;
+}
+
+StringRef NewName = Name.drop_front(PrefixPos);
+StringRef FName = Name.substr(0, PrefixPos - 1);
+if (NewName.size() == 0) {
+  return;
+}
+
+// This name should have a static linkage.
+size_t PostfixPos = NewName.find(FunctionSamples::UniqSuffix);
+bool ProfileHasFUnique = (PostfixPos != StringRef::npos);
+
+if (SampleProfileHasFUnique) {
+  // If profile also uses funqiue, nothing to do here.
+  // Otherwise, we need to build the map.
+  if (!ProfileHasFUnique) {
+std::string NStr =
+NewName.str() + getUniqueInternalLinkagePostfix(FName);
+NewName = StringRef(NStr);
+StaticFuncMap[NewName] = Name;
+return;
+  }
+} else {
+  // If profile does not use -funique-internal-linakge-symbols, nothing to
+  // do here. Otherwise, we need to trim.
+  if (ProfileHasFUnique) {
+NewName = NewName.substr(0, PostfixPos);
+  }
+}
+
+if (StaticFuncMap.find(NewName) == StaticFuncMap.end()) {
+  StaticFuncMap[NewName] = Name;
+} else {
+  StaticFuncMap[NewName] = DuplicateNameStr;
+}
+  };
+
   for (auto &PD : WC->Writer.getProfileData()) {
 // Populate IPBuilder.
 for (const auto &PDV : PD.getValue()) {
@@ -553,7 +624,9 @@
 
 // Initialize InstrProfileMap.
 InstrProfRecord *R = &PD.getValue().begin()->second;
-InstrProfileMap[PD.getKey()] = InstrProfileEntry(R);
+StringRef FullName = PD.getKey();
+InstrProfileMap[FullName] = InstrProfileEntry(R);
+buildStaticFuncMap(FullName);
   }
 
   ProfileSummary InstrPS = *IPBuilder.getSummary();
@@ -581,16 +654,27 @@
   // Find hot/warm functions in sample profile which is cold in instr profile
   // and adjust the profiles of those functions in the instr profile.
   for (const auto &PD : Reader->getProfiles()) {
-auto &FContext = PD.first;
 const sampleprof::FunctionSamples &FS = PD.second;
+if (FS.getMaxCountInside() <= ColdSampleThreshold ||
+FS.getBodySamples().size() < SupplMinSizeThreshold)
+ 

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 455682.
xur added a comment.

Fixed unneeded &


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132600/new/

https://reviews.llvm.org/D132600

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/ProfileData/SampleProf.h
  llvm/test/tools/llvm-profdata/Inputs/FUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/FUnique.proftext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.proftext
  llvm/test/tools/llvm-profdata/suppl-instr-with-sample-static-func.test
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/MD5.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ThreadPool.h"
@@ -42,6 +43,10 @@
 
 using namespace llvm;
 
+// We use this string to indicate that there are
+// multiple static functions map to the same name.
+const std::string DuplicateNameStr = "";
+
 enum ProfileFormat {
   PF_None = 0,
   PF_Text,
@@ -478,7 +483,7 @@
   ZeroCounterRatio = (float)ZeroCntNum / CntNum;
 }
 
-/// Either set all the counters in the instr profile entry \p IFE to -1
+// Either set all the counters in the instr profile entry \p IFE to -1
 /// in order to drop the profile or scale up the counters in \p IFP to
 /// be above hot threshold. We use the ratio of zero counters in the
 /// profile of a function to decide the profile is helpful or harmful
@@ -539,7 +544,73 @@
unsigned InstrProfColdThreshold) {
   // Function to its entry in instr profile.
   StringMap InstrProfileMap;
+  StringMap StaticFuncMap;
   InstrProfSummaryBuilder IPBuilder(ProfileSummaryBuilder::DefaultCutoffs);
+
+  auto checkSampleProfileHasFUnique = [&Reader]() {
+for (const auto &PD : Reader->getProfiles()) {
+  auto &FContext = PD.first;
+  if (FContext.toString().find(FunctionSamples::UniqSuffix) !=
+  std::string::npos) {
+return true;
+  }
+}
+return false;
+  };
+
+  bool SampleProfileHasFUnique = checkSampleProfileHasFUnique();
+
+  auto buildStaticFuncMap = [&StaticFuncMap,
+ SampleProfileHasFUnique](const StringRef Name) {
+std::string Prefixes[] = {".cpp:", "cc:", ".c:", ".hpp:", ".h:"};
+size_t PrefixPos = StringRef::npos;
+for (auto &Prefix : Prefixes) {
+  PrefixPos = Name.find_insensitive(Prefix);
+  if (PrefixPos == StringRef::npos)
+continue;
+  PrefixPos += Prefix.size();
+  break;
+}
+
+if (PrefixPos == StringRef::npos) {
+  return;
+}
+
+StringRef NewName = Name.drop_front(PrefixPos);
+StringRef FName = Name.substr(0, PrefixPos - 1);
+if (NewName.size() == 0) {
+  return;
+}
+
+// This name should have a static linkage.
+size_t PostfixPos = NewName.find(FunctionSamples::UniqSuffix);
+bool ProfileHasFUnique = (PostfixPos != StringRef::npos);
+
+if (SampleProfileHasFUnique) {
+  // If profile also uses funqiue, nothing to do here.
+  // Otherwise, we need to build the map.
+  if (!ProfileHasFUnique) {
+std::string NStr =
+NewName.str() + getUniqueInternalLinkagePostfix(FName);
+NewName = StringRef(NStr);
+StaticFuncMap[NewName] = Name;
+return;
+  }
+} else {
+  // If profile does not use -funique-internal-linakge-symbols, nothing to
+  // do here. Otherwise, we need to trim.
+  if (ProfileHasFUnique) {
+NewName = NewName.substr(0, PostfixPos);
+  }
+}
+
+if (StaticFuncMap.find(NewName) == StaticFuncMap.end()) {
+  StaticFuncMap[NewName] = Name;
+} else {
+  StaticFuncMap[NewName] = DuplicateNameStr;
+}
+  };
+
   for (auto &PD : WC->Writer.getProfileData()) {
 // Populate IPBuilder.
 for (const auto &PDV : PD.getValue()) {
@@ -553,7 +624,9 @@
 
 // Initialize InstrProfileMap.
 InstrProfRecord *R = &PD.getValue().begin()->second;
-InstrProfileMap[PD.getKey()] = InstrProfileEntry(R);
+StringRef FullName = PD.getKey();
+InstrProfileMap[FullName] = InstrProfileEntry(R);
+buildStaticFuncMap(FullName);
   }
 
   ProfileSummary InstrPS = *IPBuilder.getSummary();
@@ -581,16 +654,27 @@
   // Find hot/warm functions in sample profile which is cold in instr profile
   // and adjust the profiles of those functions in the instr profile.
   for (const auto &PD : Reader->getProfiles()) {
-auto &FContext = PD.first;
 const sampleprof::FunctionSamples &FS = PD.second;
+if (FS.getMaxCountInside() <= ColdSampleThreshold ||
+FS.getBodySamples().size() < SupplMinSizeThreshold)
+  continue;
+

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

[Rong Xu via Phabricator via cfe-commits]

xur added inline comments.



Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:589
+
+if (SampleProfileHasFUnique) {
+  // If profile also uses funqiue, nothing to do here.

tmsriram wrote:
> Maybe rewrite this slightly as:
> 
> //  If Sample Profile and Instrumented Profile do not agree on symbol 
> uniqification.
> if (SampleProfileHasFunique != ProfileHasFUnique) {
>if (ProfileHasFUnique) {
>   // trim
>} else {
>   // Build Map
>}
> }
Good point. I will change to this form.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132600/new/

https://reviews.llvm.org/D132600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 455816.
xur added a comment.

Integrated Sriraman's suggestion.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132600/new/

https://reviews.llvm.org/D132600

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/include/llvm/ProfileData/SampleProf.h
  llvm/test/tools/llvm-profdata/Inputs/FUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/FUnique.proftext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.afdotext
  llvm/test/tools/llvm-profdata/Inputs/NoFUnique.proftext
  llvm/test/tools/llvm-profdata/suppl-instr-with-sample-static-func.test
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/MD5.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ThreadPool.h"
@@ -42,6 +43,10 @@
 
 using namespace llvm;
 
+// We use this string to indicate that there are
+// multiple static functions map to the same name.
+const std::string DuplicateNameStr = "";
+
 enum ProfileFormat {
   PF_None = 0,
   PF_Text,
@@ -478,7 +483,7 @@
   ZeroCounterRatio = (float)ZeroCntNum / CntNum;
 }
 
-/// Either set all the counters in the instr profile entry \p IFE to -1
+// Either set all the counters in the instr profile entry \p IFE to -1
 /// in order to drop the profile or scale up the counters in \p IFP to
 /// be above hot threshold. We use the ratio of zero counters in the
 /// profile of a function to decide the profile is helpful or harmful
@@ -539,7 +544,73 @@
unsigned InstrProfColdThreshold) {
   // Function to its entry in instr profile.
   StringMap InstrProfileMap;
+  StringMap StaticFuncMap;
   InstrProfSummaryBuilder IPBuilder(ProfileSummaryBuilder::DefaultCutoffs);
+
+  auto checkSampleProfileHasFUnique = [&Reader]() {
+for (const auto &PD : Reader->getProfiles()) {
+  auto &FContext = PD.first;
+  if (FContext.toString().find(FunctionSamples::UniqSuffix) !=
+  std::string::npos) {
+return true;
+  }
+}
+return false;
+  };
+
+  bool SampleProfileHasFUnique = checkSampleProfileHasFUnique();
+
+  auto buildStaticFuncMap = [&StaticFuncMap,
+ SampleProfileHasFUnique](const StringRef Name) {
+std::string Prefixes[] = {".cpp:", "cc:", ".c:", ".hpp:", ".h:"};
+size_t PrefixPos = StringRef::npos;
+for (auto &Prefix : Prefixes) {
+  PrefixPos = Name.find_insensitive(Prefix);
+  if (PrefixPos == StringRef::npos)
+continue;
+  PrefixPos += Prefix.size();
+  break;
+}
+
+if (PrefixPos == StringRef::npos) {
+  return;
+}
+
+StringRef NewName = Name.drop_front(PrefixPos);
+StringRef FName = Name.substr(0, PrefixPos - 1);
+if (NewName.size() == 0) {
+  return;
+}
+
+// This name should have a static linkage.
+size_t PostfixPos = NewName.find(FunctionSamples::UniqSuffix);
+bool ProfileHasFUnique = (PostfixPos != StringRef::npos);
+
+// If sample profile and instrumented profile do not agree on symbol
+// uniqification.
+if (SampleProfileHasFUnique != ProfileHasFUnique) {
+  // If instrumented profile uses -funique-internal-linakge-symbols,
+  // we need to trim the name.
+  if (ProfileHasFUnique) {
+NewName = NewName.substr(0, PostfixPos);
+  } else {
+// If sample profile uses -funique-internal-linakge-symbols,
+// we build the map.
+std::string NStr =
+NewName.str() + getUniqueInternalLinkagePostfix(FName);
+NewName = StringRef(NStr);
+StaticFuncMap[NewName] = Name;
+return;
+  }
+}
+
+if (StaticFuncMap.find(NewName) == StaticFuncMap.end()) {
+  StaticFuncMap[NewName] = Name;
+} else {
+  StaticFuncMap[NewName] = DuplicateNameStr;
+}
+  };
+
   for (auto &PD : WC->Writer.getProfileData()) {
 // Populate IPBuilder.
 for (const auto &PDV : PD.getValue()) {
@@ -553,7 +624,9 @@
 
 // Initialize InstrProfileMap.
 InstrProfRecord *R = &PD.getValue().begin()->second;
-InstrProfileMap[PD.getKey()] = InstrProfileEntry(R);
+StringRef FullName = PD.getKey();
+InstrProfileMap[FullName] = InstrProfileEntry(R);
+buildStaticFuncMap(FullName);
   }
 
   ProfileSummary InstrPS = *IPBuilder.getSummary();
@@ -581,16 +654,27 @@
   // Find hot/warm functions in sample profile which is cold in instr profile
   // and adjust the profiles of those functions in the instr profile.
   for (const auto &PD : Reader->getProfiles()) {
-auto &FContext = PD.first;
 const sampleprof::FunctionSamples &FS = PD.second;
+if (FS.getMaxCountInside() <= ColdSampleThreshold ||
+FS.

[PATCH] D157750: Properly handle -fsplit-machine-functions for fatbinary compilation

[Rong Xu via Phabricator via cfe-commits]

xur accepted this revision.
xur added a comment.

lgtm


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157750/new/

https://reviews.llvm.org/D157750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62627: [NFC] Do not run CGProfilePass when not using integrated assembler

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

look good to me with the fix of comments.




Comment at: llvm/include/llvm/Passes/PassBuilder.h:110
+  /// Tuning option to enable/disable call graph profile. Its default value is
+  /// true.
+  bool CallGrpahProfile;

The default value is specified by -enable-npm-call-graph-profile, rather than 
true, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62627/new/

https://reviews.llvm.org/D62627



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

[Rong Xu via Phabricator via cfe-commits]

xur created this revision.
xur added reviewers: tejohnson, jeroen.dobbelaere.

This patch fixes PR44896.

For IR input files, option fdiscard-value-names should be ignored as we need 
named values in loadModule().
Commit 60d3947922 
 sets this 
option after loadModule() where valued names already created. This creates an 
inconsistent state in setNameImpl() that leads to a seg fault.
This patch forces fdiscard-value-names to be false for IR input files.


https://reviews.llvm.org/D74878

Files:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/test/CodeGen/PR44896.ll


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,7 @@
+; RUN: %clang_cc1 -triple x86_64-unknown-unknown -O3 -S -emit-llvm %s 
-discard-value-names -o -
+; PR 44896
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -1146,6 +1146,9 @@
CI.getTargetOpts(), CI.getLangOpts(),
CI.getFrontendOpts().ShowTimers,
std::move(LinkModules), *VMContext, nullptr);
+// PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
+// true here because the valued names are needed for reading textual IR.
+Ctx.setDiscardValueNames(false);
 Ctx.setDiagnosticHandler(
 std::make_unique(CodeGenOpts, &Result));
 


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,7 @@
+; RUN: %clang_cc1 -triple x86_64-unknown-unknown -O3 -S -emit-llvm %s -discard-value-names -o -
+; PR 44896
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -1146,6 +1146,9 @@
CI.getTargetOpts(), CI.getLangOpts(),
CI.getFrontendOpts().ShowTimers,
std::move(LinkModules), *VMContext, nullptr);
+// PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
+// true here because the valued names are needed for reading textual IR.
+Ctx.setDiscardValueNames(false);
 Ctx.setDiagnosticHandler(
 std::make_unique(CodeGenOpts, &Result));
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

In D74878#1884769 , @serge-sans-paille 
wrote:

> @lebedev.ri maybe we should at least warn the user if there was a flag at 
> clang level to force discarding? That's what I've been doing in 
> https://reviews.llvm.org/D74871?id=245594


@serge-sans-paille
It seems to me that we are not dropping the flag of -fdiscard-value-names.  
It's just the implementation of setNameImpl() that assumes the names would not 
be generated under this option.
In this sense, your patch is more complete as it breaks this removes this 
assumption.

My patch will fall back to the old behavior before Commit 60d3947922 
.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74878/new/

https://reviews.llvm.org/D74878



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 245891.
xur added reviewers: mehdi_amini, serge-sans-paille.
xur added a comment.

Update the patch based on the reviews from Mehdi and Serge.
Reuse Sege's warning code in https://reviews.llvm.org/D74871?id=245594.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74878/new/

https://reviews.llvm.org/D74878

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/PR44896.ll


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,13 @@
+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; CHECK: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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--linux-gnu"
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4332,8 +4332,15 @@
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-   options::OPT_fno_discard_value_names, !IsAssertBuild))
+   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+if (std::any_of(Inputs.begin(), Inputs.end(),
+[](const clang::driver::InputInfo &II) {
+  return types::isLLVMIR(II.getType());
+})) {
+  D.Diag(diag::warn_ignoring_fdiscard_for_bitcode);
+}
 CmdArgs.push_back("-discard-value-names");
+  }
 
   // Set the main file name, so that debug info works even with
   // -save-temps.
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -1146,6 +1146,9 @@
CI.getTargetOpts(), CI.getLangOpts(),
CI.getFrontendOpts().ShowTimers,
std::move(LinkModules), *VMContext, nullptr);
+// PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
+// true here because the valued names are needed for reading textual IR.
+Ctx.setDiscardValueNames(false);
 Ctx.setDiagnosticHandler(
 std::make_unique(CodeGenOpts, &Result));
 
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -271,6 +271,9 @@
   InGroup;
 def warn_c_kext : Warning<
   "ignoring -fapple-kext which is valid for C++ and Objective-C++ only">;
+def warn_ignoring_fdiscard_for_bitcode : Warning<
+  "ignoring -fdiscard-value-names for LLVM Bitcode">,
+  InGroup;
 def warn_drv_input_file_unused : Warning<
   "%0: '%1' input unused%select{ when '%3' is present|}2">,
   InGroup;


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,13 @@
+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; CHECK: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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--linux-gnu"
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4332,8 +4332,15 @@
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-   options::OPT_fno_discard_value_names, !IsAssertBuild))
+   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+if (std::any_of(Inputs.begin(), Inputs.end(),
+[](const clang::driver::InputInfo &II) {
+  return types::isLLVMIR(II.getType());
+})) {
+  D.Diag(diag::warn_ignoring_fdiscard_for_bitcode);
+}
 CmdArgs.push_back("-discard-value-names");
+  }
 
   // Set the main file name, so that debug info works even with
   // -save-temps.
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -1146,6 +1146,9 @@

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

Gentle ping. Is the newest patch ok?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74878/new/

https://reviews.llvm.org/D74878



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 246260.
xur added a comment.

check if the -fdiscard-value-names explicitly in args (suggested by Jeroen and 
Serge)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74878/new/

https://reviews.llvm.org/D74878

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/PR44896.ll


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,15 @@
+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck 
--check-prefix=WARNING %s
+; RUN: %clang -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=NOWARNING %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; WARNING: iignoring -fdiscard-value-names for LLVM Bitcode
+; NOWARNING-NOT: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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--linux-gnu"
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4332,8 +4332,16 @@
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-   options::OPT_fno_discard_value_names, !IsAssertBuild))
+   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+if (Args.hasArg(options::OPT_fdiscard_value_names) &&
+(std::any_of(Inputs.begin(), Inputs.end(),
+ [](const clang::driver::InputInfo &II) {
+   return types::isLLVMIR(II.getType());
+ }))) {
+  D.Diag(diag::warn_ignoring_fdiscard_for_bitcode);
+}
 CmdArgs.push_back("-discard-value-names");
+  }
 
   // Set the main file name, so that debug info works even with
   // -save-temps.
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -1146,6 +1146,9 @@
CI.getTargetOpts(), CI.getLangOpts(),
CI.getFrontendOpts().ShowTimers,
std::move(LinkModules), *VMContext, nullptr);
+// PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
+// true here because the valued names are needed for reading textual IR.
+Ctx.setDiscardValueNames(false);
 Ctx.setDiagnosticHandler(
 std::make_unique(CodeGenOpts, &Result));
 
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -271,6 +271,9 @@
   InGroup;
 def warn_c_kext : Warning<
   "ignoring -fapple-kext which is valid for C++ and Objective-C++ only">;
+def warn_ignoring_fdiscard_for_bitcode : Warning<
+  "ignoring -fdiscard-value-names for LLVM Bitcode">,
+  InGroup;
 def warn_drv_input_file_unused : Warning<
   "%0: '%1' input unused%select{ when '%3' is present|}2">,
   InGroup;


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,15 @@
+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=WARNING %s
+; RUN: %clang -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=NOWARNING %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; WARNING: iignoring -fdiscard-value-names for LLVM Bitcode
+; NOWARNING-NOT: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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--linux-gnu"
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4332,8 +4332,16 @@
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-   options::OPT_fno_discard_value_names, !IsAssertBuild))
+   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+if (Args.hasArg(options::OPT_fdiscard_value_names) &&
+(std::any_of(Inputs.begin(), Inputs.end(),
+ [](const clang::driver::InputInfo &II) {
+   return types::isLLVMIR(II.getType());
+ }))) {
+  D.Diag(diag::warn_ignoring_fdiscard_for_

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 246265.
xur added a comment.

Uploaded a wrong patch in the last time. 
This is the correct one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74878/new/

https://reviews.llvm.org/D74878

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/PR44896.ll


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,15 @@
+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck 
--check-prefix=WARNING %s
+; RUN: %clang -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=NOWARNING %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; WARNING: ignoring -fdiscard-value-names for LLVM Bitcode
+; NOWARNING-NOT: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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--linux-gnu"
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4332,8 +4332,16 @@
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-   options::OPT_fno_discard_value_names, !IsAssertBuild))
+   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+if (Args.hasArg(options::OPT_fdiscard_value_names) &&
+(std::any_of(Inputs.begin(), Inputs.end(),
+ [](const clang::driver::InputInfo &II) {
+   return types::isLLVMIR(II.getType());
+ }))) {
+  D.Diag(diag::warn_ignoring_fdiscard_for_bitcode);
+}
 CmdArgs.push_back("-discard-value-names");
+  }
 
   // Set the main file name, so that debug info works even with
   // -save-temps.
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -1146,6 +1146,9 @@
CI.getTargetOpts(), CI.getLangOpts(),
CI.getFrontendOpts().ShowTimers,
std::move(LinkModules), *VMContext, nullptr);
+// PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
+// true here because the valued names are needed for reading textual IR.
+Ctx.setDiscardValueNames(false);
 Ctx.setDiagnosticHandler(
 std::make_unique(CodeGenOpts, &Result));
 
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -271,6 +271,9 @@
   InGroup;
 def warn_c_kext : Warning<
   "ignoring -fapple-kext which is valid for C++ and Objective-C++ only">;
+def warn_ignoring_fdiscard_for_bitcode : Warning<
+  "ignoring -fdiscard-value-names for LLVM Bitcode">,
+  InGroup;
 def warn_drv_input_file_unused : Warning<
   "%0: '%1' input unused%select{ when '%3' is present|}2">,
   InGroup;


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,15 @@
+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=WARNING %s
+; RUN: %clang -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=NOWARNING %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; WARNING: ignoring -fdiscard-value-names for LLVM Bitcode
+; NOWARNING-NOT: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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--linux-gnu"
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4332,8 +4332,16 @@
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-   options::OPT_fno_discard_value_names, !IsAssertBuild))
+   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+if (Args.hasArg(options::OPT_fdiscard_value_names) &&
+(std::any_of(Inputs.begin(), Inputs.end(),
+ [](const clang::driver::InputInfo &II) {
+   return types::isLLVMIR(II.getType());
+ }))) {
+  D.Diag(diag::warn_ignoring_fdiscard_for_bitcode);
+}
 

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

[Rong Xu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG11857d49948b: [remark][diagnostics] [codegen] Fix PR44896 
(authored by xur).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74878/new/

https://reviews.llvm.org/D74878

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/PR44896.ll


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,15 @@
+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck 
--check-prefix=WARNING %s
+; RUN: %clang -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=NOWARNING %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; WARNING: ignoring -fdiscard-value-names for LLVM Bitcode
+; NOWARNING-NOT: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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--linux-gnu"
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4332,8 +4332,16 @@
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-   options::OPT_fno_discard_value_names, !IsAssertBuild))
+   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+if (Args.hasArg(options::OPT_fdiscard_value_names) &&
+(std::any_of(Inputs.begin(), Inputs.end(),
+ [](const clang::driver::InputInfo &II) {
+   return types::isLLVMIR(II.getType());
+ }))) {
+  D.Diag(diag::warn_ignoring_fdiscard_for_bitcode);
+}
 CmdArgs.push_back("-discard-value-names");
+  }
 
   // Set the main file name, so that debug info works even with
   // -save-temps.
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -1146,6 +1146,9 @@
CI.getTargetOpts(), CI.getLangOpts(),
CI.getFrontendOpts().ShowTimers,
std::move(LinkModules), *VMContext, nullptr);
+// PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
+// true here because the valued names are needed for reading textual IR.
+Ctx.setDiscardValueNames(false);
 Ctx.setDiagnosticHandler(
 std::make_unique(CodeGenOpts, &Result));
 
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -271,6 +271,9 @@
   InGroup;
 def warn_c_kext : Warning<
   "ignoring -fapple-kext which is valid for C++ and Objective-C++ only">;
+def warn_ignoring_fdiscard_for_bitcode : Warning<
+  "ignoring -fdiscard-value-names for LLVM Bitcode">,
+  InGroup;
 def warn_drv_input_file_unused : Warning<
   "%0: '%1' input unused%select{ when '%3' is present|}2">,
   InGroup;


Index: clang/test/CodeGen/PR44896.ll
===
--- /dev/null
+++ clang/test/CodeGen/PR44896.ll
@@ -0,0 +1,15 @@
+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=WARNING %s
+; RUN: %clang -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=NOWARNING %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; WARNING: ignoring -fdiscard-value-names for LLVM Bitcode
+; NOWARNING-NOT: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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--linux-gnu"
+
+define linkonce i8* @b(i8* %a) {
+  ret i8* %a
+}
+
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4332,8 +4332,16 @@
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-   options::OPT_fno_discard_value_names, !IsAssertBuild))
+   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+if (Args.hasArg(options::OPT_fdiscard_value_names) &&
+(std::any_of(Inputs.begin(), Inputs.end(),
+ [](const clang::driver::InputInfo &II) {
+   return types::isLLVMIR(II.g

[PATCH] D28867: [Profile] Warn about out-of-date profiles only when there are mismatches

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

This change gonna hide the missing profile for a function permanently if there 
is no mismatch. Like, if a user adds a new function without changing existing 
functions, he will never get a warning if using the old profiles.

In LLVM IR level PGO, we also find the missing profile warning sometime too 
verbose. We turn missing profile off by default, but not depending on mismatch. 
I think that is a better method.


https://reviews.llvm.org/D28867



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28867: [Profile] Add off-by-default -Wprofile-instr-missing warning

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

Looks good to me.


https://reviews.llvm.org/D28867



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99821: [PGO, test] Fix typo in FileCheck var

[Rong Xu via Phabricator via cfe-commits]

xur accepted this revision.
xur added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99821/new/

https://reviews.llvm.org/D99821

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94463: [Driver] Fix assertion failure when -fprofile-generate -fcs-profile-generate are used together

[Rong Xu via Phabricator via cfe-commits]

xur accepted this revision.
xur added a comment.
This revision is now accepted and ready to land.

looks good to me. Thanks for working on this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94463/new/

https://reviews.llvm.org/D94463

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108499: [clang][codegen] Don't assert on CurLinkModule for silenced diagnostic

[Rong Xu via Phabricator via cfe-commits]

xur added a comment.

This looks fine to me. But I agree you that a more complete fix would be having 
CurLinkModule set for all callers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108499/new/

https://reviews.llvm.org/D108499

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108603: [clang][codegen] Set CurLinkModule in CodeGenAction::ExecuteAction

[Rong Xu via Phabricator via cfe-commits]

xur accepted this revision.
xur added a comment.
This revision is now accepted and ready to land.

Thanks for fix this!
I like this fix and the patch looks good to me.

-Rong


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108603/new/

https://reviews.llvm.org/D108603

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits