Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

Create a new review here:
http://reviews.llvm.org/D17622

Thanks,

-Rong

On Wed, Feb 24, 2016 at 9:22 PM, Sean Silva  wrote:
> silvas added a comment.
>
> In http://reviews.llvm.org/D15829#360006, @xur wrote:
>
>> Here is the new patch that removes the auto detection of profile kind.
>>
>> In this patch, I replace the CC1 option of -fprofile-instr-use=<> with 
>> -fprofile-instrument={llvm-use|clang-use}. For the use compilation, the 
>> profile reuses the -fprofile-instrument-path= option.
>>
>> Again, all changes are of CC1 options. Driver options are intact.
>>
>> Some test are modified due to the -fprofile-instr-use= option change.
>>
>> A new test is added to test IR profile compilation.
>>  test/CodeGen/pgo-instrumentation.c
>
>
> I meant in a new phabricator review. This one is has gone on for too long and 
> is convoluted and hard to follow. (lots of stray inline comments etc.)
>
>
> http://reviews.llvm.org/D15829
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur added a comment.

Create a new review here:
http://reviews.llvm.org/D17622

Thanks,

-Rong


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added a comment.

In http://reviews.llvm.org/D15829#360006, @xur wrote:

> Here is the new patch that removes the auto detection of profile kind.
>
> In this patch, I replace the CC1 option of -fprofile-instr-use=<> with 
> -fprofile-instrument={llvm-use|clang-use}. For the use compilation, the 
> profile reuses the -fprofile-instrument-path= option.
>
> Again, all changes are of CC1 options. Driver options are intact.
>
> Some test are modified due to the -fprofile-instr-use= option change.
>
> A new test is added to test IR profile compilation. 
>  test/CodeGen/pgo-instrumentation.c


I meant in a new phabricator review. This one is has gone on for too long and 
is convoluted and hard to follow. (lots of stray inline comments etc.)


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added a comment.

Looks good to me -- and it makes the profile-gen and profile-use's cc1 option 
handling consistent.  Please check with Sean or Justin just in case before 
proceeding.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur updated this revision to Diff 48859.
xur marked 2 inline comments as done.
xur added a comment.

Integrated with David's comments.

Thanks,

-Rong


http://reviews.llvm.org/D15829

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotestclang.profraw
  test/CodeGen/Inputs/pgotestir.profraw
  test/CodeGen/pgo-instrumentation.c
  test/Driver/clang_f_opts.c
  test/Profile/c-captured.c
  test/Profile/c-counter-overflows.c
  test/Profile/c-general.c
  test/Profile/c-outdated-data.c
  test/Profile/c-unprofiled-blocks.c
  test/Profile/c-unprofiled.c
  test/Profile/cxx-lambda.cpp
  test/Profile/cxx-rangefor.cpp
  test/Profile/cxx-templates.cpp
  test/Profile/objc-general.m
  test/Profile/profile-does-not-exist.c

Index: test/Profile/profile-does-not-exist.c
===
--- test/Profile/profile-does-not-exist.c
+++ test/Profile/profile-does-not-exist.c
@@ -1,4 +1,4 @@
-// RUN: not %clang_cc1 -emit-llvm %s -o - -fprofile-instr-use=%t.nonexistent.profdata 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -emit-llvm %s -o - -fprofile-instrument-path=%t.nonexistent.profdata -fprofile-instrument=clang-use 2>&1 | FileCheck %s
 
 // CHECK: error: Could not read profile {{.*}}.nonexistent.profdata:
 // CHECK-NOT: Assertion failed
Index: test/Profile/objc-general.m
===
--- test/Profile/objc-general.m
+++ test/Profile/objc-general.m
@@ -3,7 +3,7 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instrument=clang | FileCheck -check-prefix=PGOGEN %s
 
 // RUN: llvm-profdata merge %S/Inputs/objc-general.proftext -o %t.profdata
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instr-use=%t.profdata | FileCheck -check-prefix=PGOUSE %s
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instrument-path=%t.profdata -fprofile-instrument=clang-use | FileCheck -check-prefix=PGOUSE %s
 
 #ifdef HAVE_FOUNDATION
 
Index: test/Profile/cxx-templates.cpp
===
--- test/Profile/cxx-templates.cpp
+++ test/Profile/cxx-templates.cpp
@@ -6,7 +6,7 @@
 // RUN: FileCheck --input-file=%tgen -check-prefix=T100GEN -check-prefix=ALL %s
 
 // RUN: llvm-profdata merge %S/Inputs/cxx-templates.proftext -o %t.profdata
-// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instr-use=%t.profdata > %tuse
+// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument-path=%t.profdata -fprofile-instrument=clang-use > %tuse
 // RUN: FileCheck --input-file=%tuse -check-prefix=T0USE -check-prefix=ALL %s
 // RUN: FileCheck --input-file=%tuse -check-prefix=T100USE -check-prefix=ALL %s
 
Index: test/Profile/cxx-rangefor.cpp
===
--- test/Profile/cxx-rangefor.cpp
+++ test/Profile/cxx-rangefor.cpp
@@ -4,7 +4,7 @@
 // RUN: FileCheck --input-file=%tgen -check-prefix=CHECK -check-prefix=PGOGEN %s
 
 // RUN: llvm-profdata merge %S/Inputs/cxx-rangefor.proftext -o %t.profdata
-// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-rangefor.cpp -std=c++11 -o - -emit-llvm -fprofile-instr-use=%t.profdata > %tuse
+// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-rangefor.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument-path=%t.profdata -fprofile-instrument=clang-use > %tuse
 // RUN: FileCheck --input-file=%tuse -check-prefix=CHECK -check-prefix=PGOUSE %s
 
 // PGOGEN: @[[RFC:__profc__Z9range_forv]] = private global [5 x i64] zeroinitializer
Index: test/Profile/cxx-lambda.cpp
===
--- test/Profile/cxx-lambda.cpp
+++ test/Profile/cxx-lambda.cpp
@@ -5,7 +5,7 @@
 // RUN: FileCheck --input-file=%tgen -check-prefix=LMBGEN %s
 
 // RUN: llvm-profdata merge %S/Inputs/cxx-lambda.proftext -o %t.profdata
-// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-lambda.cpp -std=c++11 -o - -emit-llvm -fprofile-instr-use=%t.profdata > %tuse
+// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-lambda.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument-path=%t.profdata -fprofile-instrument=clang-use > %tuse
 // RUN: FileCheck --input-file=%tuse -check-prefix=PGOUSE %s
 // RUN: FileCheck --input-file=%tuse -check-prefix=LMBUSE %s
 
Index: test/Profile/c-unprofiled.c

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur marked 4 inline comments as done.


Comment at: lib/CodeGen/CodeGenModule.cpp:151
@@ -151,1 +150,3 @@
+  if (CodeGenOpts.hasProfileClangUse() &&
+  !CodeGenOpts.InstrProfileInput.empty()) {
 auto ReaderOrErr =

davidxl wrote:
> Is the second condition still needed? Should it be asserted ?
The second condition is not needed. I'll change the code in the coming patch.


Comment at: test/CodeGen/pgo-instrumentation.c:4
@@ +3,3 @@
+// 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
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass

davidxl wrote:
> should be %clang_cc1
I think both %clang_cc1 and %clang-cc1 are accepted. But all the other tests 
are using %clang_cc1. I'll change.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

Here is the new patch that removes the auto detection of profile kind.

In this patch, I replace the CC1 option of -fprofile-instr-use=<> with 
-fprofile-instrument={llvm-use|clang-use}. For the use compilation, the profile 
reuses the -fprofile-instrument-path= option.

Again, all changes are of CC1 options. Driver options are intact.

Some test are modified due to the -fprofile-instr-use= option change.

A new test is added to test IR profile compilation. 
test/CodeGen/pgo-instrumentation.c


http://reviews.llvm.org/D15829

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotestclang.profraw
  test/CodeGen/Inputs/pgotestir.profraw
  test/CodeGen/pgo-instrumentation.c
  test/Driver/clang_f_opts.c
  test/Profile/c-captured.c
  test/Profile/c-counter-overflows.c
  test/Profile/c-general.c
  test/Profile/c-outdated-data.c
  test/Profile/c-unprofiled-blocks.c
  test/Profile/c-unprofiled.c
  test/Profile/cxx-lambda.cpp
  test/Profile/cxx-rangefor.cpp
  test/Profile/cxx-templates.cpp
  test/Profile/objc-general.m
  test/Profile/profile-does-not-exist.c

Index: test/Profile/profile-does-not-exist.c
===
--- test/Profile/profile-does-not-exist.c
+++ test/Profile/profile-does-not-exist.c
@@ -1,4 +1,4 @@
-// RUN: not %clang_cc1 -emit-llvm %s -o - -fprofile-instr-use=%t.nonexistent.profdata 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -emit-llvm %s -o - -fprofile-instrument-path=%t.nonexistent.profdata -fprofile-instrument=clang-use 2>&1 | FileCheck %s
 
 // CHECK: error: Could not read profile {{.*}}.nonexistent.profdata:
 // CHECK-NOT: Assertion failed
Index: test/Profile/objc-general.m
===
--- test/Profile/objc-general.m
+++ test/Profile/objc-general.m
@@ -3,7 +3,7 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instrument=clang | FileCheck -check-prefix=PGOGEN %s
 
 // RUN: llvm-profdata merge %S/Inputs/objc-general.proftext -o %t.profdata
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instr-use=%t.profdata | FileCheck -check-prefix=PGOUSE %s
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instrument-path=%t.profdata -fprofile-instrument=clang-use | FileCheck -check-prefix=PGOUSE %s
 
 #ifdef HAVE_FOUNDATION
 
Index: test/Profile/cxx-templates.cpp
===
--- test/Profile/cxx-templates.cpp
+++ test/Profile/cxx-templates.cpp
@@ -6,7 +6,7 @@
 // RUN: FileCheck --input-file=%tgen -check-prefix=T100GEN -check-prefix=ALL %s
 
 // RUN: llvm-profdata merge %S/Inputs/cxx-templates.proftext -o %t.profdata
-// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instr-use=%t.profdata > %tuse
+// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument-path=%t.profdata -fprofile-instrument=clang-use > %tuse
 // RUN: FileCheck --input-file=%tuse -check-prefix=T0USE -check-prefix=ALL %s
 // RUN: FileCheck --input-file=%tuse -check-prefix=T100USE -check-prefix=ALL %s
 
Index: test/Profile/cxx-rangefor.cpp
===
--- test/Profile/cxx-rangefor.cpp
+++ test/Profile/cxx-rangefor.cpp
@@ -4,7 +4,7 @@
 // RUN: FileCheck --input-file=%tgen -check-prefix=CHECK -check-prefix=PGOGEN %s
 
 // RUN: llvm-profdata merge %S/Inputs/cxx-rangefor.proftext -o %t.profdata
-// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-rangefor.cpp -std=c++11 -o - -emit-llvm -fprofile-instr-use=%t.profdata > %tuse
+// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-rangefor.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument-path=%t.profdata -fprofile-instrument=clang-use > %tuse
 // RUN: FileCheck --input-file=%tuse -check-prefix=CHECK -check-prefix=PGOUSE %s
 
 // PGOGEN: @[[RFC:__profc__Z9range_forv]] = private global [5 x i64] zeroinitializer
Index: test/Profile/cxx-lambda.cpp
===
--- test/Profile/cxx-lambda.cpp
+++ test/Profile/cxx-lambda.cpp
@@ -5,7 +5,7 @@
 // RUN: FileCheck --input-file=%tgen -check-prefix=LMBGEN %s
 
 // RUN: llvm-profdata merge %S/Inputs/cxx-lambda.proftext -o %t.profdata
-// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

I'll send a revised patch soon.



Comment at: lib/CodeGen/CodeGenModule.cpp:150
@@ -149,2 +149,3 @@
 
-  if (!CodeGenOpts.InstrProfileInput.empty()) {
+  if (!CodeGenOpts.hasProfileIRInstr() &&
+  !CodeGenOpts.InstrProfileInput.empty()) {

davidxl wrote:
> Better to use if (CodeGenOpts.hasProfileClangInstr() && ..)
OK. That would require to insert an extra cc1 option of 
-profile-instrument=clang for profile-use. And possibly some test case option 
changes too.


Comment at: lib/Driver/Tools.cpp:3201
@@ +3200,3 @@
+// Set the profile kind if it's not the default clang kind.
+static void setProfileKindFlag(ArgStringList ,
+   std::string ProfileName) {

davidxl wrote:
> I don't quite like this change in the driver. I think the right thing to do 
> is:
> 
> 1) for profile use case, there is no need to pass -fprofile-instrument=<...> 
> FE option from the driver
> 2) The profileInstrKind determination needs to happen in FE (not driver) by 
> peeking into the passed in profile data.
I'll split off the profile kind detection to a separated patch suggested by 
Sean.


Comment at: test/CodeGen/pgo-instrumentation.c:4
@@ +3,3 @@
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm 
-fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s 
-check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass

silvas wrote:
> Use `%clang_cc1` here and elsewhere.
Will do.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added a comment.

Apologies for the delay. At this point, I think that this patch has evolved 
enough that it is best to start a new patch. I think the steps forward are:

- Have cc1 accept -fprofile-instrument=llvm (requires no driver changes, but is 
enough for developers to test by being careful and manually passing it)
- Start a discussion on llvm-dev to decide on the driver-level
- implement other driver features as discussed
  - e.g. if we decide that -fprofile-instr-use= should detect FE/IR (i.e. IR is 
not a separate flag), add FE/IR detection for -fprofile-instr-use=



Comment at: test/CodeGen/pgo-instrumentation.c:4
@@ +3,3 @@
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm 
-fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s 
-check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass

Use `%clang_cc1` here and elsewhere.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added inline comments.


Comment at: lib/CodeGen/CodeGenModule.cpp:150
@@ -149,2 +149,3 @@
 
-  if (!CodeGenOpts.InstrProfileInput.empty()) {
+  if (!CodeGenOpts.hasProfileIRInstr() &&
+  !CodeGenOpts.InstrProfileInput.empty()) {

Better to use if (CodeGenOpts.hasProfileClangInstr() && ..)


Comment at: lib/Driver/Tools.cpp:3201
@@ +3200,3 @@
+// Set the profile kind if it's not the default clang kind.
+static void setProfileKindFlag(ArgStringList ,
+   std::string ProfileName) {

I don't quite like this change in the driver. I think the right thing to do is:

1) for profile use case, there is no need to pass -fprofile-instrument=<...> FE 
option from the driver
2) The profileInstrKind determination needs to happen in FE (not driver) by 
peeking into the passed in profile data.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

Ping

(also added the test that missed from last upload).

Thanks,

-Rong


http://reviews.llvm.org/D15829

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/CMakeLists.txt
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotestclang.profraw
  test/CodeGen/Inputs/pgotestir.profraw
  test/CodeGen/pgo-instrumentation.c

Index: test/CodeGen/pgo-instrumentation.c
===
--- test/CodeGen/pgo-instrumentation.c
+++ test/CodeGen/pgo-instrumentation.c
@@ -0,0 +1,28 @@
+// Test if PGO instrumentation and use pass are invoked.
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm -fprofile-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-GEN
+// CHECK-PGOGENPASS-INVOKED-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=clang -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NOT: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotestir.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm -fprofile-instr-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-INSTR-USE
+// CHECK-PGOUSEPASS-INVOKED-INSTR-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotestir.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE
+// CHECK-PGOUSEPASS-INVOKED-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is not invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotestclang.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=clang -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE-CLANG
+// CHECK-PGOUSEPASS-INVOKED-USE-CLANG-NOT: PGOInstrumentationUsePass
Index: test/CodeGen/Inputs/pgotestir.profraw
===
--- test/CodeGen/Inputs/pgotestir.profraw
+++ test/CodeGen/Inputs/pgotestir.profraw
@@ -0,0 +1 @@
+:ir
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -478,14 +478,18 @@
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
 
-  enum PGOInstrumentor { Unknown, None, Clang };
+  enum PGOInstrumentor { Unknown, None, Clang, LLVM };
   if (Arg *A = Args.getLastArg(OPT_fprofile_instrument_EQ)) {
 StringRef Value = A->getValue();
 PGOInstrumentor Method = llvm::StringSwitch(Value)
- .Case("clang", Clang)
  .Case("none", None)
+ .Case("clang", Clang)
+ .Case("llvm", LLVM)
  .Default(Unknown);
 switch (Method) {
+case LLVM:
+  Opts.setProfileInstr(CodeGenOptions::ProfileIRInstr);
+  break;
 case Clang:
   Opts.setProfileInstr(CodeGenOptions::ProfileClangInstr);
   break;
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -32,6 +32,7 @@
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
+#include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -3196,6 +3197,18 @@
   return VersionTuple();
 }
 
+// Set the profile kind if it's not the default clang kind.
+static void setProfileKindFlag(ArgStringList ,
+   std::string ProfileName) {
+  auto ReaderOrErr = llvm::IndexedInstrProfReader::create(ProfileName);
+  if (ReaderOrErr.getError())
+return;
+  std::unique_ptr PGOReader =
+std::move(ReaderOrErr.get());
+  

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

fixed the typo in comments


http://reviews.llvm.org/D15829

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/CMakeLists.txt
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp

Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -478,14 +478,18 @@
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
 
-  enum PGOInstrumentor { Unknown, None, Clang };
+  enum PGOInstrumentor { Unknown, None, Clang, LLVM };
   if (Arg *A = Args.getLastArg(OPT_fprofile_instrument_EQ)) {
 StringRef Value = A->getValue();
 PGOInstrumentor Method = llvm::StringSwitch(Value)
- .Case("clang", Clang)
  .Case("none", None)
+ .Case("clang", Clang)
+ .Case("llvm", LLVM)
  .Default(Unknown);
 switch (Method) {
+case LLVM:
+  Opts.setProfileInstr(CodeGenOptions::ProfileIRInstr);
+  break;
 case Clang:
   Opts.setProfileInstr(CodeGenOptions::ProfileClangInstr);
   break;
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -32,6 +32,7 @@
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
+#include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -3196,6 +3197,18 @@
   return VersionTuple();
 }
 
+// Set the profile kind if it's not the default clang kind.
+static void setProfileKindFlag(ArgStringList ,
+   std::string ProfileName) {
+  auto ReaderOrErr = llvm::IndexedInstrProfReader::create(ProfileName);
+  if (ReaderOrErr.getError())
+return;
+  std::unique_ptr PGOReader =
+std::move(ReaderOrErr.get());
+  if (PGOReader->isIRLevelProfile())
+CmdArgs.push_back("-fprofile-instrument=llvm");
+}
+
 static void addPGOAndCoverageFlags(Compilation , const Driver ,
const InputInfo , const ArgList ,
ArgStringList ) {
@@ -3238,8 +3251,10 @@
   }
 
   if (ProfileUseArg) {
-if (ProfileUseArg->getOption().matches(options::OPT_fprofile_instr_use_EQ))
+if (ProfileUseArg->getOption().matches(options::OPT_fprofile_instr_use_EQ)) {
   ProfileUseArg->render(Args, CmdArgs);
+  setProfileKindFlag(CmdArgs, ProfileUseArg->getValue());
+}
 else if ((ProfileUseArg->getOption().matches(
   options::OPT_fprofile_use_EQ) ||
   ProfileUseArg->getOption().matches(
@@ -3250,6 +3265,7 @@
 llvm::sys::path::append(Path, "default.profdata");
   CmdArgs.push_back(
   Args.MakeArgString(Twine("-fprofile-instr-use=") + Path));
+  setProfileKindFlag(CmdArgs, Path.str().str());
 }
   }
 
Index: lib/Driver/CMakeLists.txt
===
--- lib/Driver/CMakeLists.txt
+++ lib/Driver/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(LLVM_LINK_COMPONENTS
   Option
   Support
+  ProfileData
   )
 
 add_clang_library(clangDriver
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -147,7 +147,8 @@
   if (C.getLangOpts().ObjC1)
 ObjCData = new ObjCEntrypoints();
 
-  if (!CodeGenOpts.InstrProfileInput.empty()) {
+  if (!CodeGenOpts.hasProfileIRInstr() &&
+  !CodeGenOpts.InstrProfileInput.empty()) {
 auto ReaderOrErr =
 llvm::IndexedInstrProfReader::create(CodeGenOpts.InstrProfileInput);
 if (std::error_code EC = ReaderOrErr.getError()) {
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -434,6 +434,13 @@
 Options.NoRedZone = CodeGenOpts.DisableRedZone;
 Options.InstrProfileOutput = CodeGenOpts.InstrProfileOutput;
 MPM->add(createInstrProfilingPass(Options));
+  } else if (CodeGenOpts.hasProfileIRInstr()) {
+if (!CodeGenOpts.InstrProfileInput.empty())
+  PMBuilder.PGOInstrUse = CodeGenOpts.InstrProfileInput;
+else if (!CodeGenOpts.InstrProfileOutput.empty())
+  PMBuilder.PGOInstrGen = CodeGenOpts.InstrProfileOutput;
+else
+  PMBuilder.PGOInstrGen = "default.profraw";
   }
 
   if (!CodeGenOpts.SampleProfileFile.empty())
Index: include/clang/Frontend/CodeGenOptions.h

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added inline comments.


Comment at: include/clang/Frontend/CodeGenOptions.h:234
@@ +233,3 @@
+
+  /// \brief Check if IR profile instrumenation is on.
+  bool hasProfileIRInstr() const {

typo: instrumentation


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

Now http://reviews.llvm.org/D16730 has been committed (as r259811)

Here is the patch that adds cc1 option -fprofile-instrument=llvm to enable IR 
level PGO generate and use.

The pgo use part of the patch depends http://reviews.llvm.org/D15540 which 
differentiates the IR level and clang profiles. To detect the profile 
automatically, we need to first the profile. It can either done in driver or in 
Clang codegen (lib/CodeGen/CodeGenModule.cpp). If we do this in Clang codegen, 
we would have to change the Codegen option which does not seem to be a good 
approach. As a result, we do this in the driver in this patch.

Again like in http://reviews.llvm.org/D16730, this patch won't change any 
driver (user level) options. Only cc1 options are touched.

Thanks,

-Rong


http://reviews.llvm.org/D15829

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/CMakeLists.txt
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotestclang.profraw
  test/CodeGen/Inputs/pgotestir.profraw
  test/CodeGen/pgo-instrumentation.c

Index: test/CodeGen/pgo-instrumentation.c
===
--- test/CodeGen/pgo-instrumentation.c
+++ test/CodeGen/pgo-instrumentation.c
@@ -0,0 +1,28 @@
+// Test if PGO instrumentation and use pass are invoked.
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm -fprofile-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-GEN
+// CHECK-PGOGENPASS-INVOKED-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=clang -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NOT: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotestir.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm -fprofile-instr-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-INSTR-USE
+// CHECK-PGOUSEPASS-INVOKED-INSTR-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotestir.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=llvm -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE
+// CHECK-PGOUSEPASS-INVOKED-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is not invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotestclang.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=clang -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE-CLANG
+// CHECK-PGOUSEPASS-INVOKED-USE-CLANG-NOT: PGOInstrumentationUsePass
Index: test/CodeGen/Inputs/pgotestir.profraw
===
--- test/CodeGen/Inputs/pgotestir.profraw
+++ test/CodeGen/Inputs/pgotestir.profraw
@@ -0,0 +1 @@
+:ir
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -478,14 +478,18 @@
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
 
-  enum PGOInstrumentor { Unknown, None, Clang };
+  enum PGOInstrumentor { Unknown, None, Clang, LLVM };
   if (Arg *A = Args.getLastArg(OPT_fprofile_instrument_EQ)) {
 StringRef Value = A->getValue();
 PGOInstrumentor Method = llvm::StringSwitch(Value)
- .Case("clang", Clang)
  .Case("none", None)
+ .Case("clang", Clang)
+ .Case("llvm", LLVM)
  .Default(Unknown);
 switch (Method) {
+case LLVM:
+  Opts.setProfileInstr(CodeGenOptions::ProfileIRInstr);
+  break;
 case Clang:
   Opts.setProfileInstr(CodeGenOptions::ProfileClangInstr);
   break;
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Wed, Feb 3, 2016 at 12:40 PM, Bob Wilson  wrote:
>
> On Feb 3, 2016, at 12:23 PM, Xinliang David Li  wrote:
>
> On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson  wrote:
>
>
> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits
>  wrote:
>
> silvas added a comment.
>
> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>
> For the longer term, one possible solution is to make FE based
> instrumentation only used for coverage testing which can be turned on
> with -fcoverage-mapping option (currently, -fcoverage-mapping can not
> be used alone and must be used together with
> -fprofile-instr-generate). To summarize:
>
> A. Current behavior:
>
> ---
>
> 1. -fprofile-instr-generate turns on FE based instrumentation
> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based
> instrumentation and coverage mapping data generation.
> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>
> B. Proposed new behavior:
>
> 
>
> 1. -fprofile-instr-generate turns on IR late instrumentation
> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
> 4. -fprofile-instr-use=<> will automatically determine how to use the
> profile data.
>
>
>
> Very good observation that we can determine FE or IR automatically based on
> the input profdata. That simplifies things.
>
> B.2) above can be done today for improved usability.
>
>
>
> I don't see how this improves usability. In fact, it is confusing because it
> hijacks an existing option.
>
>
> Hijacking an existing option to do something different would definitely be a
> problem. Please find a way to specify IR-level instrumentation without
> breaking compatibility. If you want to replace the existing options with
> something different, we’ll need a transition period of at least 1-2 LLVM
> releases to migrate.
>
>
> If we remove B.3 above,  then the proposed change (B.2) is essentially
> making '-fcoverage-mapping' an alias to '-fprofile-instr-generate
> -fcoverage-mapping'.   No existing workflow will be broken and new
> users can take advantage of the shortened option.  The reason is that
> there will be no users that only use -fcoverage-mapping option alone
> and rely on its behavior (which is clang error).
>
>
> The part I’m concerned about is B.1. The current behavior is that
> -fprofile-instr-generate enabled FE instrumentation. We can’t hijack that to
> do something different, at least without a sufficiently long transition
> period for clients to adapt. We use that to generate PGO profiles even when
> not using -fcoverage-mapping.

yes. I don't see this happening overnight. IR based instrumentation
also needs to get stablized and widely used before the switch can
happen.

>>
> Yes, that is a requirement for us. We need existing profdata to work with
> newer versions of clang (which is why IR-level instrumentation doesn’t work
> for us).
>
>
> profile-use can automatically detect FE based profile data and use it
> properly. The question is whether we have a need to support merging
> profiles from IR and FE instrumentation.
>
>
> I don’t think it makes sense to merge those. They seem like fundamentally
> different kinds of data. The “forward compatibility” requirement is about
> different versions of the FE-based profile data.


great -- one fewer thing to worry about.

>

>
> I want to understand how we can guarantee to support old (FE based)
> profiles with new compilers.  The region to counter id
> mapping/assignment depends on how the AST is generated by the frontend
> and how the AST is traversed. Do we have any guarantee that the new
> compiler can generate them in the same order? How is that enforced?
> The function structural hash generated may also be different (given
> the same source).
>
>
> The FE-based instrumentation uses a custom traversal of the ASTs so that we
> can control the order and make sure it doesn’t change. It still depends on
> the way the ASTs are generated but the AST nodes that are relevant for this
> are unlikely to change in ways that would affect the instrumentation. I
> would love to have a better way to enforce that.

My guess is that IR based instrumentation (which is CFG based) can
produce as stable profile as what FE based instrumentation (when it is
used in a conservative mode that does not require pre-inlining).  CFG
based checksum is also used to detect incompatible changes such that
old profile data can be kept live for a long time (while
gradually/slowly degrades in quality due to the increased number of
mismatches).To get really stable profile, we need to used a source
location based representation (which sample based PGO uses).In
other words, I don't see a big obstacle that prevent IR based
instrumentation from being usable  in such a workflow.

>
> The 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson  wrote:
>
>> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits 
>>  wrote:
>>
>> silvas added a comment.
>>
>> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>>
>>> For the longer term, one possible solution is to make FE based
>>> instrumentation only used for coverage testing which can be turned on
>>> with -fcoverage-mapping option (currently, -fcoverage-mapping can not
>>> be used alone and must be used together with
>>> -fprofile-instr-generate). To summarize:
>>>
>>> A. Current behavior:
>>>
>>> ---
>>>
>>> 1. -fprofile-instr-generate turns on FE based instrumentation
>>> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
>>> instrumentation and coverage mapping data generation.
>>> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>>>
>>> B. Proposed new behavior:
>>>
>>> 
>>>
>>> 1. -fprofile-instr-generate turns on IR late instrumentation
>>> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
>>> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
>>> 4. -fprofile-instr-use=<> will automatically determine how to use the 
>>> profile data.
>>
>>
>> Very good observation that we can determine FE or IR automatically based on 
>> the input profdata. That simplifies things.
>>
>>> B.2) above can be done today for improved usability.
>>
>>
>> I don't see how this improves usability. In fact, it is confusing because it 
>> hijacks an existing option.
>
> Hijacking an existing option to do something different would definitely be a 
> problem. Please find a way to specify IR-level instrumentation without 
> breaking compatibility. If you want to replace the existing options with 
> something different, we’ll need a transition period of at least 1-2 LLVM 
> releases to migrate.
>

If we remove B.3 above,  then the proposed change (B.2) is essentially
making '-fcoverage-mapping' an alias to '-fprofile-instr-generate
-fcoverage-mapping'.   No existing workflow will be broken and new
users can take advantage of the shortened option.  The reason is that
there will be no users that only use -fcoverage-mapping option alone
and rely on its behavior (which is clang error).


>>
>> Also B.3 causes existing user builds to emit a warning, which is annoying.
>>
>> I would propose the following modification of B:
>>
>> C.:
>>
>> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
>> exactly as before, except that it uses IR instrumentation)
>> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
>> instrumentation and coverage. (i.e. behaves exactly as before)
>> 3. -fprofile-instr-use=<> automatically determines which method to use
>>
>> All existing user workflows continue to work, except for workflows that 
>> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they 
>> have checked-in to version control and represents some special workload) 
>> with the profile data from new binaries.
>
> The coverage mapping adds considerable cost. IR-level instrumentation has 
> some problems that make it undesirable for our workflow, so we need a way to 
> select front-end instrumentation without adding a bunch of unnecessary 
> overhead (generating the coverage mapping when you’re not actually doing 
> coverage testing). I disagree with your assessment that existing workflows 
> would continue to “work” because ours would not.
>
>>
>> Concretely, imagine the following workflow:
>>
>>  clang -fprofile-instr-generate foo.c -o foo
>>  ./foo
>>  llvm-profdata merge default.profraw -o new.profdata
>>  llvm-profdata merge new.profdata 
>> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
>> -o foo.profdata
>>  clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
>>
>> I think this is a reasonable breakage. We would need to add a note in the 
>> release notes. Unfortunately this is not expected breakage if we claim to 
>> have forward compatibility for profdata (which IIRC Apple requires; 
>> @bogner?).
>
> Yes, that is a requirement for us. We need existing profdata to work with 
> newer versions of clang (which is why IR-level instrumentation doesn’t work 
> for us).
>

profile-use can automatically detect FE based profile data and use it
properly. The question is whether we have a need to support merging
profiles from IR and FE instrumentation.



>> But I think this case will be rare and exceptional enough that we can 
>> tolerate it:
>>
>> - a simple immediate workaround is to specify `-fcoverage-mapping` (which 
>> also adds some extra stuff, but works around the issue)
>> - Presumably 
>> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
>> is regenerated with some frequency which is more frequent than upgrading the 
>> compiler, and so it is likely reasonable to regenerate it 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Bob Wilson via cfe-commits]

> On Feb 3, 2016, at 12:23 PM, Xinliang David Li  wrote:
> 
> On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson  > wrote:
>> 
>>> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits 
>>>  wrote:
>>> 
>>> silvas added a comment.
>>> 
>>> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>>> 
 For the longer term, one possible solution is to make FE based
 instrumentation only used for coverage testing which can be turned on
 with -fcoverage-mapping option (currently, -fcoverage-mapping can not
 be used alone and must be used together with
 -fprofile-instr-generate). To summarize:
 
 A. Current behavior:
 
 ---
 
 1. -fprofile-instr-generate turns on FE based instrumentation
 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
 instrumentation and coverage mapping data generation.
 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
 
 B. Proposed new behavior:
 
 
 
 1. -fprofile-instr-generate turns on IR late instrumentation
 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
 4. -fprofile-instr-use=<> will automatically determine how to use the 
 profile data.
>>> 
>>> 
>>> Very good observation that we can determine FE or IR automatically based on 
>>> the input profdata. That simplifies things.
>>> 
 B.2) above can be done today for improved usability.
>>> 
>>> 
>>> I don't see how this improves usability. In fact, it is confusing because 
>>> it hijacks an existing option.
>> 
>> Hijacking an existing option to do something different would definitely be a 
>> problem. Please find a way to specify IR-level instrumentation without 
>> breaking compatibility. If you want to replace the existing options with 
>> something different, we’ll need a transition period of at least 1-2 LLVM 
>> releases to migrate.
>> 
> 
> If we remove B.3 above,  then the proposed change (B.2) is essentially
> making '-fcoverage-mapping' an alias to '-fprofile-instr-generate
> -fcoverage-mapping'.   No existing workflow will be broken and new
> users can take advantage of the shortened option.  The reason is that
> there will be no users that only use -fcoverage-mapping option alone
> and rely on its behavior (which is clang error).

The part I’m concerned about is B.1. The current behavior is that 
-fprofile-instr-generate enabled FE instrumentation. We can’t hijack that to do 
something different, at least without a sufficiently long transition period for 
clients to adapt. We use that to generate PGO profiles even when not using 
-fcoverage-mapping.

> 
> 
>>> 
>>> Also B.3 causes existing user builds to emit a warning, which is annoying.
>>> 
>>> I would propose the following modification of B:
>>> 
>>> C.:
>>> 
>>> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
>>> exactly as before, except that it uses IR instrumentation)
>>> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
>>> instrumentation and coverage. (i.e. behaves exactly as before)
>>> 3. -fprofile-instr-use=<> automatically determines which method to use
>>> 
>>> All existing user workflows continue to work, except for workflows that 
>>> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they 
>>> have checked-in to version control and represents some special workload) 
>>> with the profile data from new binaries.
>> 
>> The coverage mapping adds considerable cost. IR-level instrumentation has 
>> some problems that make it undesirable for our workflow, so we need a way to 
>> select front-end instrumentation without adding a bunch of unnecessary 
>> overhead (generating the coverage mapping when you’re not actually doing 
>> coverage testing). I disagree with your assessment that existing workflows 
>> would continue to “work” because ours would not.
>> 
>>> 
>>> Concretely, imagine the following workflow:
>>> 
>>> clang -fprofile-instr-generate foo.c -o foo
>>> ./foo
>>> llvm-profdata merge default.profraw -o new.profdata
>>> llvm-profdata merge new.profdata 
>>> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
>>> -o foo.profdata
>>> clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
>>> 
>>> I think this is a reasonable breakage. We would need to add a note in the 
>>> release notes. Unfortunately this is not expected breakage if we claim to 
>>> have forward compatibility for profdata (which IIRC Apple requires; 
>>> @bogner?).
>> 
>> Yes, that is a requirement for us. We need existing profdata to work with 
>> newer versions of clang (which is why IR-level instrumentation doesn’t work 
>> for us).
>> 
> 
> profile-use can automatically detect FE based profile data and use it
> 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Bob Wilson via cfe-commits]

> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits 
>  wrote:
> 
> silvas added a comment.
> 
> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
> 
>> For the longer term, one possible solution is to make FE based
>> instrumentation only used for coverage testing which can be turned on
>> with -fcoverage-mapping option (currently, -fcoverage-mapping can not
>> be used alone and must be used together with
>> -fprofile-instr-generate). To summarize:
>> 
>> A. Current behavior:
>> 
>> ---
>> 
>> 1. -fprofile-instr-generate turns on FE based instrumentation
>> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
>> instrumentation and coverage mapping data generation.
>> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>> 
>> B. Proposed new behavior:
>> 
>> 
>> 
>> 1. -fprofile-instr-generate turns on IR late instrumentation
>> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
>> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
>> 4. -fprofile-instr-use=<> will automatically determine how to use the 
>> profile data.
> 
> 
> Very good observation that we can determine FE or IR automatically based on 
> the input profdata. That simplifies things.
> 
>> B.2) above can be done today for improved usability.
> 
> 
> I don't see how this improves usability. In fact, it is confusing because it 
> hijacks an existing option.

Hijacking an existing option to do something different would definitely be a 
problem. Please find a way to specify IR-level instrumentation without breaking 
compatibility. If you want to replace the existing options with something 
different, we’ll need a transition period of at least 1-2 LLVM releases to 
migrate.

> 
> Also B.3 causes existing user builds to emit a warning, which is annoying.
> 
> I would propose the following modification of B:
> 
> C.:
> 
> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
> exactly as before, except that it uses IR instrumentation)
> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
> instrumentation and coverage. (i.e. behaves exactly as before)
> 3. -fprofile-instr-use=<> automatically determines which method to use
> 
> All existing user workflows continue to work, except for workflows that 
> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they 
> have checked-in to version control and represents some special workload) with 
> the profile data from new binaries.

The coverage mapping adds considerable cost. IR-level instrumentation has some 
problems that make it undesirable for our workflow, so we need a way to select 
front-end instrumentation without adding a bunch of unnecessary overhead 
(generating the coverage mapping when you’re not actually doing coverage 
testing). I disagree with your assessment that existing workflows would 
continue to “work” because ours would not.

> 
> Concretely, imagine the following workflow:
> 
>  clang -fprofile-instr-generate foo.c -o foo
>  ./foo
>  llvm-profdata merge default.profraw -o new.profdata
>  llvm-profdata merge new.profdata 
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
> -o foo.profdata
>  clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
> 
> I think this is a reasonable breakage. We would need to add a note in the 
> release notes. Unfortunately this is not expected breakage if we claim to 
> have forward compatibility for profdata (which IIRC Apple requires; @bogner?).

Yes, that is a requirement for us. We need existing profdata to work with newer 
versions of clang (which is why IR-level instrumentation doesn’t work for us).

> But I think this case will be rare and exceptional enough that we can 
> tolerate it:
> 
> - a simple immediate workaround is to specify `-fcoverage-mapping` (which 
> also adds some extra stuff, but works around the issue)
> - Presumably 
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
> is regenerated with some frequency which is more frequent than upgrading the 
> compiler, and so it is likely reasonable to regenerate it alongside a 
> compiler upgrade, using the workaround until then.

No, that assumption is not necessarily true for us. We need to be able to 
upgrade the compiler without breaking projects that we don’t control, and that 
includes regressing their performance because of an outdated profile.

> 
> 
> 
>> B.1) needs a
> 
>> transition period before  the IR based instrumentation becomes
> 
>> stablized (and can be flipped to the default).  During the transition
> 
>> period, the behavior of 1) does not change, but a cc1 option can be
> 
>> used to turn on IR instrumentation (as proposed by Sean).
> 
> 
> Just to clarify, users are not allowed to use cc1 options. The cc1 option is 
> purely for us as compiler developers to do integration and 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

This new patch adds the change the clang test cases (cc1 option 
-fprofile-instr-generate to -fprofile-instrument=Clang).

It also replaces cc1 option -fprofile-instr-generate= to 
-fprofile-instrument-path=, as suggested by David.

Thanks,

-Rong


http://reviews.llvm.org/D15829

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGStmt.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenPGO.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotest.profraw
  test/CodeGen/pgo-instrumentation.c
  test/CoverageMapping/block-storage-starts-region.m
  test/CoverageMapping/break.c
  test/CoverageMapping/builtinmacro.c
  test/CoverageMapping/casts.c
  test/CoverageMapping/classtemplate.cpp
  test/CoverageMapping/comment-in-macro.c
  test/CoverageMapping/continue.c
  test/CoverageMapping/control-flow-macro.c
  test/CoverageMapping/decl.c
  test/CoverageMapping/header.cpp
  test/CoverageMapping/if.c
  test/CoverageMapping/implicit-def-in-macro.m
  test/CoverageMapping/includehell.cpp
  test/CoverageMapping/ir.c
  test/CoverageMapping/label.cpp
  test/CoverageMapping/lambda.cpp
  test/CoverageMapping/logical.cpp
  test/CoverageMapping/loopmacro.c
  test/CoverageMapping/loops.cpp
  test/CoverageMapping/macro-expansion.c
  test/CoverageMapping/macro-expressions.cpp
  test/CoverageMapping/macroception.c
  test/CoverageMapping/macroparams.c
  test/CoverageMapping/macroparams2.c
  test/CoverageMapping/macros.c
  test/CoverageMapping/macroscopes.cpp
  test/CoverageMapping/md.cpp
  test/CoverageMapping/moremacros.c
  test/CoverageMapping/nestedclass.cpp
  test/CoverageMapping/objc.m
  test/CoverageMapping/preprocessor.c
  test/CoverageMapping/return.c
  test/CoverageMapping/switch.c
  test/CoverageMapping/switchmacro.c
  test/CoverageMapping/system_macro.c
  test/CoverageMapping/templates.cpp
  test/CoverageMapping/test.c
  test/CoverageMapping/trycatch.cpp
  test/CoverageMapping/trymacro.cpp
  test/CoverageMapping/unreachable-macro.c
  test/CoverageMapping/unused_names.c
  test/CoverageMapping/while.c
  test/Driver/clang_f_opts.c
  test/Profile/c-captured.c
  test/Profile/c-general.c
  test/Profile/c-generate.c
  test/Profile/c-indirect-call.c
  test/Profile/c-linkage-available_externally.c
  test/Profile/c-linkage.c
  test/Profile/c-unreachable-after-switch.c
  test/Profile/cxx-implicit.cpp
  test/Profile/cxx-lambda.cpp
  test/Profile/cxx-linkage.cpp
  test/Profile/cxx-rangefor.cpp
  test/Profile/cxx-structors.cpp
  test/Profile/cxx-templates.cpp
  test/Profile/cxx-virtual-destructor-calls.cpp
  test/Profile/objc-general.m

Index: test/Profile/objc-general.m
===
--- test/Profile/objc-general.m
+++ test/Profile/objc-general.m
@@ -1,6 +1,6 @@
 // Test instrumentation of general constructs in objective C.
 
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instr-generate | FileCheck -check-prefix=PGOGEN %s
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instrument=Clang | FileCheck -check-prefix=PGOGEN %s
 
 // RUN: llvm-profdata merge %S/Inputs/objc-general.proftext -o %t.profdata
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name objc-general.m %s -o - -emit-llvm -fblocks -fprofile-instr-use=%t.profdata | FileCheck -check-prefix=PGOUSE %s
Index: test/Profile/cxx-virtual-destructor-calls.cpp
===
--- test/Profile/cxx-virtual-destructor-calls.cpp
+++ test/Profile/cxx-virtual-destructor-calls.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm -main-file-name cxx-virtual-destructor-calls.cpp %s -o - -fprofile-instr-generate | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm -main-file-name cxx-virtual-destructor-calls.cpp %s -o - -fprofile-instrument=Clang | FileCheck %s
 
 struct Member {
   ~Member();
Index: test/Profile/cxx-templates.cpp
===
--- test/Profile/cxx-templates.cpp
+++ test/Profile/cxx-templates.cpp
@@ -1,7 +1,7 @@
 // Tests for instrumentation of templated code. Each instantiation of a template
 // should be instrumented separately.
 
-// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instr-generate > %tgen
+// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument=Clang > %tgen
 // RUN: 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur added a comment.

To make the review easier, I split the cc1 names change part into a new NFC 
patch here.
http://reviews.llvm.org/D16730


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added inline comments.


Comment at: lib/Driver/Tools.cpp:3232
@@ -3231,2 +3231,3 @@
   CmdArgs.push_back(
   Args.MakeArgString(Twine("-fprofile-instr-generate=") + Path));
+}

I also suggest changing CC1 option -fprofile-instr-generate= to be a different 
name (from the  driver option): -fprofile-instrument-path=... or something.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

The previous patch was not good as it failed 58 tests that use
-fprofile-instr-generate as a cc1 option.

I can see two methods to solve this:
(1) we change all the 58 tests to use -fprofile-instrument=Clang option.
(2) Fall back to my previous patch: keep -fprofile-instr-generate as
the CC1 options and check it in Frontend/CompilerInvocation.cpp. (i.e.
The redundant else branch Sean was referring to).  We have to do it
here because we cannot assume driver will append
-fprofile-instrument=Clang in the case the user uses
-fprofile-instr-generate as the cc1 option.



On Thu, Jan 28, 2016 at 12:04 PM, Rong Xu  wrote:
> xur updated this revision to Diff 46303.
> xur added a comment.
>
> Integrated most recently comments/suggestions from David and Sean.
>
> Thanks,
>
> -Rong
>
>
> http://reviews.llvm.org/D15829
>
> Files:
>   include/clang/Basic/DiagnosticDriverKinds.td
>   include/clang/Driver/CC1Options.td
>   include/clang/Driver/Options.td
>   include/clang/Frontend/CodeGenOptions.def
>   include/clang/Frontend/CodeGenOptions.h
>   lib/CodeGen/BackendUtil.cpp
>   lib/CodeGen/CGStmt.cpp
>   lib/CodeGen/CodeGenFunction.cpp
>   lib/CodeGen/CodeGenFunction.h
>   lib/CodeGen/CodeGenModule.cpp
>   lib/CodeGen/CodeGenPGO.cpp
>   lib/Driver/Tools.cpp
>   lib/Frontend/CompilerInvocation.cpp
>   test/CodeGen/Inputs/pgotest.profraw
>   test/CodeGen/pgo-instrumentation.c
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Thu, Jan 28, 2016 at 2:34 PM, Rong Xu  wrote:
> The previous patch was not good as it failed 58 tests that use
> -fprofile-instr-generate as a cc1 option.
>
> I can see two methods to solve this:
> (1) we change all the 58 tests to use -fprofile-instrument=Clang option.

My vote is on (1) -- get rid of -fprofile-instr-generate as cc1 option
as it is now a subset of what -fprofile-instrument=<...> does.

Changing test cases are mechanical.

David

> (2) Fall back to my previous patch: keep -fprofile-instr-generate as
> the CC1 options and check it in Frontend/CompilerInvocation.cpp. (i.e.
> The redundant else branch Sean was referring to).  We have to do it
> here because we cannot assume driver will append
> -fprofile-instrument=Clang in the case the user uses
> -fprofile-instr-generate as the cc1 option.


>
>
>
> On Thu, Jan 28, 2016 at 12:04 PM, Rong Xu  wrote:
>> xur updated this revision to Diff 46303.
>> xur added a comment.
>>
>> Integrated most recently comments/suggestions from David and Sean.
>>
>> Thanks,
>>
>> -Rong
>>
>>
>> http://reviews.llvm.org/D15829
>>
>> Files:
>>   include/clang/Basic/DiagnosticDriverKinds.td
>>   include/clang/Driver/CC1Options.td
>>   include/clang/Driver/Options.td
>>   include/clang/Frontend/CodeGenOptions.def
>>   include/clang/Frontend/CodeGenOptions.h
>>   lib/CodeGen/BackendUtil.cpp
>>   lib/CodeGen/CGStmt.cpp
>>   lib/CodeGen/CodeGenFunction.cpp
>>   lib/CodeGen/CodeGenFunction.h
>>   lib/CodeGen/CodeGenModule.cpp
>>   lib/CodeGen/CodeGenPGO.cpp
>>   lib/Driver/Tools.cpp
>>   lib/Frontend/CompilerInvocation.cpp
>>   test/CodeGen/Inputs/pgotest.profraw
>>   test/CodeGen/pgo-instrumentation.c
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur added a comment.

The previous patch was not good as it failed 58 tests that use
-fprofile-instr-generate as a cc1 option.

I can see two methods to solve this:
(1) we change all the 58 tests to use -fprofile-instrument=Clang option.
(2) Fall back to my previous patch: keep -fprofile-instr-generate as
the CC1 options and check it in Frontend/CompilerInvocation.cpp. (i.e.
The redundant else branch Sean was referring to).  We have to do it
here because we cannot assume driver will append
-fprofile-instrument=Clang in the case the user uses
-fprofile-instr-generate as the cc1 option.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

Integrated most recently comments/suggestions from David and Sean.

Thanks,

-Rong


http://reviews.llvm.org/D15829

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGStmt.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenPGO.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotest.profraw
  test/CodeGen/pgo-instrumentation.c

Index: test/CodeGen/pgo-instrumentation.c
===
--- /dev/null
+++ test/CodeGen/pgo-instrumentation.c
@@ -0,0 +1,28 @@
+// Test if PGO instrumentation and use pass are invoked.
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=LLVM -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=LLVM -fprofile-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-GEN
+// CHECK-PGOGENPASS-INVOKED-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=Clang -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NOT: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=LLVM -fprofile-instr-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-INSTR-USE
+// CHECK-PGOUSEPASS-INVOKED-INSTR-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=LLVM -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE
+// CHECK-PGOUSEPASS-INVOKED-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is not invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrument=Clang -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE-CLANG
+// CHECK-PGOUSEPASS-INVOKED-USE-CLANG-NOT: PGOInstrumentationUsePass
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -477,8 +477,29 @@
   Opts.DisableIntegratedAS = Args.hasArg(OPT_fno_integrated_as);
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
-  Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
-  Args.hasArg(OPT_fprofile_instr_generate_EQ);
+
+  enum PGOInstrumentor { Unknown, Clang, LLVM };
+  if (Arg *A = Args.getLastArg(OPT_fprofile_instrument_EQ)) {
+StringRef Value = A->getValue();
+PGOInstrumentor Method = llvm::StringSwitch(Value)
+ .Case("Clang", Clang)
+ .Case("LLVM", LLVM)
+ .Default(Unknown);
+switch (Method) {
+case LLVM:
+  Opts.setProfileInstr(CodeGenOptions::ProfileIRInstr);
+  break;
+case Clang:
+  Opts.setProfileInstr(CodeGenOptions::ProfileClangInstr);
+  break;
+case Unknown:
+default:
+  Diags.Report(diag::err_drv_invalid_pgo_instrumentor)
+  << A->getAsString(Args) << Value;
+  break;
+}
+  }
+
   Opts.InstrProfileOutput = Args.getLastArgValue(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileInput = Args.getLastArgValue(OPT_fprofile_instr_use_EQ);
   Opts.CoverageMapping =
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -3230,8 +3230,9 @@
   llvm::sys::path::append(Path, "default.profraw");
   CmdArgs.push_back(
   Args.MakeArgString(Twine("-fprofile-instr-generate=") + Path));
-} else
-  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_instr_generate);
+}
+// The default is to use Clang Instrumentation.
+CmdArgs.push_back("-fprofile-instrument=Clang");

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur added a comment.

I'll send an updated patch shortly.



Comment at: include/clang/Frontend/CodeGenOptions.def:106
@@ -105,3 +105,3 @@
 
-CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate
-///< execution counts to use with PGO.
+CODEGENOPT(ProfileClangInstr, 1, 0) ///< Clang Instrumentation to generate
+///< execution counts to use with PGO.

silvas wrote:
> davidxl wrote:
> > For the sake of being consistent with the new cc1 option, making this an 
> > enum option is better:
> > 
> > ENUM_CODEGENOPT(ProfileInstr,... ) which takes two legal values 
> > ProfInstrClang, ProfInstrLLVM
> > 
> > I'd like to hear Sean's opinion on this.
> SGTM. I was going to suggest something similar. This is more consistent with 
> e.g. OPT_debug_info_kind_EQ and the other multi-value options.
OK. I'll use ENUM as you two suggested.


Comment at: lib/CodeGen/CodeGenModule.cpp:150
@@ -149,3 +149,3 @@
 
-  if (!CodeGenOpts.InstrProfileInput.empty()) {
+  if (!CodeGenOpts.ProfileIRInstr && !CodeGenOpts.InstrProfileInput.empty()) {
 auto ReaderOrErr =

silvas wrote:
> This seems like a latent bug: `!CodeGenOpts.ProfileIRInstr` is true when we 
> are doing no instrumentation. Using an ENUM_CODEGENOPT will fix this and 
> related issues.
I don't think this is a bug. As I mentioned in the update comments. I still use 
this option (-fprofile-instrumentor=) in profile-use compilation. Once we have 
the llvm-profdata patch in, we can detect the profile kind and remove this.


Comment at: lib/Frontend/CompilerInvocation.cpp:495
@@ +494,3 @@
+}
+  } else {
+// Default PGO instrumentor is Clang instrumentation.

silvas wrote:
> The work being done in this `else` is redundant. 
> `-fprofile-instrumentor={clang,llvm,none}` (with "none" being the default if 
> the argument isn't passed) covers this case as 
> `-fprofile-instrumentor=clang`. The driver can be changed to translate 
> `-fprofile-instr-generate` into `-fprofile-instrumentor=clang`. (and 
> `-fprofile-instrument=` might be a better name)
Only if we changed the driver and pushed in -fprofile-instrumentor to the cc1 
argument list, the else part is indeed redundant.

But since I did not change the driver options handling, we still need this else 
branch -- we only set ProfileClangInstr when there is 
OPT_fprofile_instr_generate.

I'll change the -fprofile-instrumentor to -fprofile-instrument and change the 
driver to push in this argument. I will probably remove 
OPT_fprfoile_instr_generate as a CC1 option.




http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added inline comments.


Comment at: include/clang/Frontend/CodeGenOptions.def:106
@@ -105,3 +105,3 @@
 
-CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate
-///< execution counts to use with PGO.
+CODEGENOPT(ProfileClangInstr, 1, 0) ///< Clang Instrumentation to generate
+///< execution counts to use with PGO.

For the sake of being consistent with the new cc1 option, making this an enum 
option is better:

ENUM_CODEGENOPT(ProfileInstr,... ) which takes two legal values ProfInstrClang, 
ProfInstrLLVM

I'd like to hear Sean's opinion on this.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added inline comments.


Comment at: lib/Frontend/CompilerInvocation.cpp:483
@@ +482,3 @@
+Opts.ProfileIRInstr = true;
+  else
+// Opts.ClangProfInstrGen will be used for Clang instrumentation only.

davidxl wrote:
> silvas wrote:
> > This still isn't factored right. I think at this point the meaning of the 
> > driver-level options is not really useful at CC1 level (too convoluted) for 
> > it to be useful to pass them through.
> > 
> > The right thing to do here is probably more like:
> > - refactor so that we have 4 individual CC1 options for InstrProfileOutput, 
> > InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably 
> > rename ClangProfInstrGen and ProfileIRInstr to be consistent with each 
> > other, e.g. "ProfileIRInstr" and "ProfileClangInstr").
> > - add logic in Driver to convert from the driver-level options to the CC1 
> > options.
> It is already pretty close to your proposal -- the only missing thing is cc1 
> option for ClangProfInstrGen. However given that IR and Clang InstrGen are 
> mutually exclusive, is an additional CC1 option needed?
There are 3 possibilities:
- clang instr
- ir instr
- no instr

A simple binary flag can only encode 2. So some sort of extra information needs 
to be passed. Off the top of my head, I thought of just adding an extra flag, 
but I like your suggestion `-fprofile-instrumentor=[clang|LLVM]`. That is clean 
and avoids having to deal with an error for the "clang instr + ir instr" case.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

This new version implemented the usage model David proposed. We now have a cc1 
option -fprofile-instrumentor={llvm | clang} to choose which instrumentation to 
use. The slight difference from David's proposal is that we can use this option 
with -fprofile-instr-use.

Thanks,

-Rong


http://reviews.llvm.org/D15829

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGStmt.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenPGO.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotest.profraw
  test/CodeGen/pgo-instrumentation.c

Index: test/CodeGen/pgo-instrumentation.c
===
--- /dev/null
+++ test/CodeGen/pgo-instrumentation.c
@@ -0,0 +1,28 @@
+// Test if PGO instrumentation and use pass are invoked.
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=llvm -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=llvm -fprofile-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-GEN
+// CHECK-PGOGENPASS-INVOKED-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=clang -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NOT: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=llvm -fprofile-instr-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-INSTR-USE
+// CHECK-PGOUSEPASS-INVOKED-INSTR-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=llvm -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE
+// CHECK-PGOUSEPASS-INVOKED-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is not invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=clang -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE-CLANG
+// CHECK-PGOUSEPASS-INVOKED-USE-CLANG-NOT: PGOInstrumentationUsePass
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -477,8 +477,28 @@
   Opts.DisableIntegratedAS = Args.hasArg(OPT_fno_integrated_as);
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
-  Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
-  Args.hasArg(OPT_fprofile_instr_generate_EQ);
+
+  enum PGOInstrumentor { Unknown, Clang, LLVM };
+  if (Arg *A = Args.getLastArg(OPT_fprofile_instrumentor_EQ)) {
+StringRef Value = A->getValue();
+PGOInstrumentor Method = llvm::StringSwitch(Value)
+  .Case("clang", Clang)
+  .Case("llvm", LLVM)
+  .Default(Unknown);
+if (Method == Unknown)
+  Diags.Report(diag::err_drv_invalid_pgo_instrumentor) 
+ << A->getAsString(Args) << Value;
+else {
+  Opts.ProfileIRInstr = (Method == LLVM);
+  Opts.ProfileClangInstr = (Method == Clang);
+}
+  } else {
+// Default PGO instrumentor is Clang instrumentation.
+Opts.ProfileClangInstr = Args.hasArg(OPT_fprofile_instr_generate) ||
+ Args.hasArg(OPT_fprofile_instr_generate_EQ);
+Opts.ProfileIRInstr = false;
+  }
+
   Opts.InstrProfileOutput = Args.getLastArgValue(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileInput = Args.getLastArgValue(OPT_fprofile_instr_use_EQ);
   Opts.CoverageMapping =
Index: lib/CodeGen/CodeGenPGO.cpp
===
--- lib/CodeGen/CodeGenPGO.cpp
+++ lib/CodeGen/CodeGenPGO.cpp
@@ -37,7 +37,7 @@
   PGOReader ? PGOReader->getVersion() : llvm::IndexedInstrProf::Version);
 
   // If we're generating a profile, create a variable for the name.
-  if 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added inline comments.


Comment at: include/clang/Frontend/CodeGenOptions.def:106
@@ -105,3 +105,3 @@
 
-CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate
-///< execution counts to use with PGO.
+CODEGENOPT(ProfileClangInstr, 1, 0) ///< Clang Instrumentation to generate
+///< execution counts to use with PGO.

davidxl wrote:
> For the sake of being consistent with the new cc1 option, making this an enum 
> option is better:
> 
> ENUM_CODEGENOPT(ProfileInstr,... ) which takes two legal values 
> ProfInstrClang, ProfInstrLLVM
> 
> I'd like to hear Sean's opinion on this.
SGTM. I was going to suggest something similar. This is more consistent with 
e.g. OPT_debug_info_kind_EQ and the other multi-value options.


Comment at: lib/CodeGen/CodeGenModule.cpp:150
@@ -149,3 +149,3 @@
 
-  if (!CodeGenOpts.InstrProfileInput.empty()) {
+  if (!CodeGenOpts.ProfileIRInstr && !CodeGenOpts.InstrProfileInput.empty()) {
 auto ReaderOrErr =

This seems like a latent bug: `!CodeGenOpts.ProfileIRInstr` is true when we are 
doing no instrumentation. Using an ENUM_CODEGENOPT will fix this and related 
issues.


Comment at: lib/Frontend/CompilerInvocation.cpp:495
@@ +494,3 @@
+}
+  } else {
+// Default PGO instrumentor is Clang instrumentation.

The work being done in this `else` is redundant. 
`-fprofile-instrumentor={clang,llvm,none}` (with "none" being the default if 
the argument isn't passed) covers this case as `-fprofile-instrumentor=clang`. 
The driver can be changed to translate `-fprofile-instr-generate` into 
`-fprofile-instrumentor=clang`. (and `-fprofile-instrument=` might be a better 
name)


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added inline comments.


Comment at: lib/Driver/Tools.cpp:5520
@@ +5519,3 @@
+A->claim();
+if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
+(Args.hasArg(options::OPT_fprofile_instr_generate) ||

silvas wrote:
> This is definitely not the right thing to do. Don't hijack -Xclang (which is 
> a completely generic thing).
Why do we need special handling here ? will the existing behavior (simple 
forwarding) work?

Note that intention of the cc1 option is to hide it from the user but make it 
used by the developers -- so we can make assumption that this option is used 
only when -fprofile-instr-generate[=...] is specified. You can add diagnostics 
later during cc1 option processing if it is not the case.

Also think about making it easier to flip the default behavior in the future, 
it might be better to make the cc1 option look like this:

-fprofile-instrumentor=[clang|LLVM]

if the option is missing, the current default will be 'clang'. In the future 
just switch it to 'llvm'.


Comment at: lib/Frontend/CompilerInvocation.cpp:483
@@ +482,3 @@
+Opts.ProfileIRInstr = true;
+  else
+// Opts.ClangProfInstrGen will be used for Clang instrumentation only.

silvas wrote:
> This still isn't factored right. I think at this point the meaning of the 
> driver-level options is not really useful at CC1 level (too convoluted) for 
> it to be useful to pass them through.
> 
> The right thing to do here is probably more like:
> - refactor so that we have 4 individual CC1 options for InstrProfileOutput, 
> InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably 
> rename ClangProfInstrGen and ProfileIRInstr to be consistent with each other, 
> e.g. "ProfileIRInstr" and "ProfileClangInstr").
> - add logic in Driver to convert from the driver-level options to the CC1 
> options.
It is already pretty close to your proposal -- the only missing thing is cc1 
option for ClangProfInstrGen. However given that IR and Clang InstrGen are 
mutually exclusive, is an additional CC1 option needed?


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

Sean: Adding a new CC1 option ProfileClangInstr will make things
cleaner. But this won't solve the problem. The root of all the mess is
there is no driver level option for IR instrumentation. I need to
either "hijack" the -Xclang option, or move the logic to
CompilerInvocation.cpp, which both you don't like.

The reason is I have to reply on the Driver option
-fprofile-instr-generate to have the right link line for the profile
library. -fprofile-instr-generate will set the Instrumentation to
Clang (regardless use of current cc1 option of
-fprofile-instr-generate, or the new proposed -fprofile-clang-instr).
For IR instrumentation where the user specifies "-Xclang
-fprofile-ir-instr", I need to overwrite the driver level option. To
get that, I either parse the -Xclang value (this is "hijack), or I
handle it in CC1 (in CompilerInvocation.cpp). I don't see a way to
avoid it.

Can we use a hidden driver option here for IR instrumentation?

On Tue, Jan 26, 2016 at 5:01 PM, Sean Silva  wrote:
> silvas added inline comments.
>
> 
> Comment at: lib/Driver/Tools.cpp:5520
> @@ +5519,3 @@
> +A->claim();
> +if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
> +(Args.hasArg(options::OPT_fprofile_instr_generate) ||
> 
> This is definitely not the right thing to do. Don't hijack -Xclang (which is 
> a completely generic thing).
>
> 
> Comment at: lib/Frontend/CompilerInvocation.cpp:483
> @@ +482,3 @@
> +Opts.ProfileIRInstr = true;
> +  else
> +// Opts.ClangProfInstrGen will be used for Clang instrumentation only.
> 
> This still isn't factored right. I think at this point the meaning of the 
> driver-level options is not really useful at CC1 level (too convoluted) for 
> it to be useful to pass them through.
>
> The right thing to do here is probably more like:
> - refactor so that we have 4 individual CC1 options for InstrProfileOutput, 
> InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably 
> rename ClangProfInstrGen and ProfileIRInstr to be consistent with each other, 
> e.g. "ProfileIRInstr" and "ProfileClangInstr").
> - add logic in Driver to convert from the driver-level options to the CC1 
> options.
>
> 
> Comment at: test/CodeGen/pgo-instrumentation.c:5
> @@ +4,3 @@
> +// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr -fprofile-instr-generate %s 
> -mllvm -debug-pass=Structure 2>&1 | FileCheck %s 
> -check-prefix=CHECK-PGOGENPASS-INVOKED-V1
> +// CHECK-PGOGENPASS-INVOKED-V1: PGOInstrumentationGenPass
> +//
> 
> It isn't clear what V1/V2 mean.
>
>
> http://reviews.llvm.org/D15829
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

On Wed, Jan 27, 2016 at 11:31 AM, David Li  wrote:
> davidxl added inline comments.
>
> 
> Comment at: lib/Driver/Tools.cpp:5520
> @@ +5519,3 @@
> +A->claim();
> +if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
> +(Args.hasArg(options::OPT_fprofile_instr_generate) ||
> 
> silvas wrote:
>> This is definitely not the right thing to do. Don't hijack -Xclang (which is 
>> a completely generic thing).
> Why do we need special handling here ? will the existing behavior (simple 
> forwarding) work?

The original patch does the simple forwarding. In that case, we handle
the cc1 options (choosing b/w IR or clang instrumentation) in
CompilerInvocation.cpp. Sean and Chad think this logic should be
driver.

>
> Note that intention of the cc1 option is to hide it from the user but make it 
> used by the developers -- so we can make assumption that this option is used 
> only when -fprofile-instr-generate[=...] is specified. You can add 
> diagnostics later during cc1 option processing if it is not the case.
>
> Also think about making it easier to flip the default behavior in the future, 
> it might be better to make the cc1 option look like this:
>
> -fprofile-instrumentor=[clang|LLVM]
>
> if the option is missing, the current default will be 'clang'. In the future 
> just switch it to 'llvm'.
>
> 
> Comment at: lib/Frontend/CompilerInvocation.cpp:483
> @@ +482,3 @@
> +Opts.ProfileIRInstr = true;
> +  else
> +// Opts.ClangProfInstrGen will be used for Clang instrumentation only.
> 
> silvas wrote:
>> This still isn't factored right. I think at this point the meaning of the 
>> driver-level options is not really useful at CC1 level (too convoluted) for 
>> it to be useful to pass them through.
>>
>> The right thing to do here is probably more like:
>> - refactor so that we have 4 individual CC1 options for InstrProfileOutput, 
>> InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably 
>> rename ClangProfInstrGen and ProfileIRInstr to be consistent with each 
>> other, e.g. "ProfileIRInstr" and "ProfileClangInstr").
>> - add logic in Driver to convert from the driver-level options to the CC1 
>> options.
> It is already pretty close to your proposal -- the only missing thing is cc1 
> option for ClangProfInstrGen. However given that IR and Clang InstrGen are 
> mutually exclusive, is an additional CC1 option needed?
>
>
> http://reviews.llvm.org/D15829
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

We first need to nail the use model of the option, and then talk about
implementation. To clarify, I think the following should be the way:

(assuming the current clang instrumetnation is the default):

1) To turn on clang based instrumentation:
  -fprofile-instr-generate[=]
2) To turn on clang based instrumentation explicitly:
  -fprofile-instr-generate[=] -Xclang -fprofile-instrumentor=clang

3) To turn on IR based instrumentation:
  -fprofile-instr-generate[=] -Xclang -fprofile-instrumentor=llvm


There is no need to change driver handling -- just let it forward. In
CompilerInvocation.cpp, diagnostics can be emitted if user does

-Xclang -fprofile-instrumentor=xxx without specfiying -fprofile-instr-generate


Handling of -fprofile-instr-use=<> is different which does not depend
on -fprofile-instrumentor, so it should be done in a different patch.

David




On Wed, Jan 27, 2016 at 11:38 AM, Rong Xu  wrote:
> On Wed, Jan 27, 2016 at 11:31 AM, David Li  wrote:
>> davidxl added inline comments.
>>
>> 
>> Comment at: lib/Driver/Tools.cpp:5520
>> @@ +5519,3 @@
>> +A->claim();
>> +if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
>> +(Args.hasArg(options::OPT_fprofile_instr_generate) ||
>> 
>> silvas wrote:
>>> This is definitely not the right thing to do. Don't hijack -Xclang (which 
>>> is a completely generic thing).
>> Why do we need special handling here ? will the existing behavior (simple 
>> forwarding) work?
>
> The original patch does the simple forwarding. In that case, we handle
> the cc1 options (choosing b/w IR or clang instrumentation) in
> CompilerInvocation.cpp. Sean and Chad think this logic should be
> driver.
>
>>
>> Note that intention of the cc1 option is to hide it from the user but make 
>> it used by the developers -- so we can make assumption that this option is 
>> used only when -fprofile-instr-generate[=...] is specified. You can add 
>> diagnostics later during cc1 option processing if it is not the case.
>>
>> Also think about making it easier to flip the default behavior in the 
>> future, it might be better to make the cc1 option look like this:
>>
>> -fprofile-instrumentor=[clang|LLVM]
>>
>> if the option is missing, the current default will be 'clang'. In the future 
>> just switch it to 'llvm'.
>>
>> 
>> Comment at: lib/Frontend/CompilerInvocation.cpp:483
>> @@ +482,3 @@
>> +Opts.ProfileIRInstr = true;
>> +  else
>> +// Opts.ClangProfInstrGen will be used for Clang instrumentation only.
>> 
>> silvas wrote:
>>> This still isn't factored right. I think at this point the meaning of the 
>>> driver-level options is not really useful at CC1 level (too convoluted) for 
>>> it to be useful to pass them through.
>>>
>>> The right thing to do here is probably more like:
>>> - refactor so that we have 4 individual CC1 options for InstrProfileOutput, 
>>> InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably 
>>> rename ClangProfInstrGen and ProfileIRInstr to be consistent with each 
>>> other, e.g. "ProfileIRInstr" and "ProfileClangInstr").
>>> - add logic in Driver to convert from the driver-level options to the CC1 
>>> options.
>> It is already pretty close to your proposal -- the only missing thing is cc1 
>> option for ClangProfInstrGen. However given that IR and Clang InstrGen are 
>> mutually exclusive, is an additional CC1 option needed?
>>
>>
>> http://reviews.llvm.org/D15829
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added inline comments.


Comment at: lib/Driver/Tools.cpp:5520
@@ +5519,3 @@
+A->claim();
+if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
+(Args.hasArg(options::OPT_fprofile_instr_generate) ||

This is definitely not the right thing to do. Don't hijack -Xclang (which is a 
completely generic thing).


Comment at: lib/Frontend/CompilerInvocation.cpp:483
@@ +482,3 @@
+Opts.ProfileIRInstr = true;
+  else
+// Opts.ClangProfInstrGen will be used for Clang instrumentation only.

This still isn't factored right. I think at this point the meaning of the 
driver-level options is not really useful at CC1 level (too convoluted) for it 
to be useful to pass them through.

The right thing to do here is probably more like:
- refactor so that we have 4 individual CC1 options for InstrProfileOutput, 
InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably rename 
ClangProfInstrGen and ProfileIRInstr to be consistent with each other, e.g. 
"ProfileIRInstr" and "ProfileClangInstr").
- add logic in Driver to convert from the driver-level options to the CC1 
options.


Comment at: test/CodeGen/pgo-instrumentation.c:5
@@ +4,3 @@
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr -fprofile-instr-generate %s 
-mllvm -debug-pass=Structure 2>&1 | FileCheck %s 
-check-prefix=CHECK-PGOGENPASS-INVOKED-V1
+// CHECK-PGOGENPASS-INVOKED-V1: PGOInstrumentationGenPass
+//

It isn't clear what V1/V2 mean.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Fri, Jan 22, 2016 at 1:43 PM, Sean Silva  wrote:
> silvas added a comment.
>
> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>
>> For the longer term, one possible solution is to make FE based
>>  instrumentation only used for coverage testing which can be turned on
>>  with -fcoverage-mapping option (currently, -fcoverage-mapping can not
>>  be used alone and must be used together with
>>  -fprofile-instr-generate). To summarize:
>>
>> A. Current behavior:
>>
>>  ---
>>
>> 1. -fprofile-instr-generate turns on FE based instrumentation
>> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
>> instrumentation and coverage mapping data generation.
>> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>>
>> B. Proposed new behavior:
>>
>>  
>>
>> 1. -fprofile-instr-generate turns on IR late instrumentation
>> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
>> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
>> 4. -fprofile-instr-use=<> will automatically determine how to use the 
>> profile data.
>
>
> Very good observation that we can determine FE or IR automatically based on 
> the input profdata. That simplifies things.
>
>> B.2) above can be done today for improved usability.
>
>
> I don't see how this improves usability. In fact, it is confusing because it 
> hijacks an existing option.
>
> Also B.3 causes existing user builds to emit a warning, which is annoying.

Since it is pointless to specify -fcoverage-mapping option alone, why
not do not automatically? Yes it means some behavior change (in a good
way) and  some annoying warnings initially, but those are trivially
fixable by the user.


>
> I would propose the following modification of B:
>
> C.:
>
> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
> exactly as before, except that it uses IR instrumentation)
> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
> instrumentation and coverage. (i.e. behaves exactly as before)

I am fine with this -- as long as the user does not need to bear the
burden of understanding where and how the instrumentation is done.

> 3. -fprofile-instr-use=<> automatically determines which method to use
>
> All existing user workflows continue to work, except for workflows that 
> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they 
> have checked-in to version control and represents some special workload) with 
> the profile data from new binaries.
>
> Concretely, imagine the following workflow:
>
>   clang -fprofile-instr-generate foo.c -o foo
>   ./foo
>   llvm-profdata merge default.profraw -o new.profdata
>   llvm-profdata merge new.profdata 
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
> -o foo.profdata
>   clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
>
> I think this is a reasonable breakage. We would need to add a note in the 
> release notes. Unfortunately this is not expected breakage if we claim to 
> have forward compatibility for profdata (which IIRC Apple requires; 
> @bogner?). But I think this case will be rare and exceptional enough that we 
> can tolerate it:
>

The old profile data has to out live the program source versions which
usually change fast. In reality, I don't expect profile data to live
too long.

> - a simple immediate workaround is to specify `-fcoverage-mapping` (which 
> also adds some extra stuff, but works around the issue)

This is a reasonable workaround

> - Presumably 
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
> is regenerated with some frequency which is more frequent than upgrading the 
> compiler, and so it is likely reasonable to regenerate it alongside a 
> compiler upgrade, using the workaround until then.
>
>
>
>> B.1) needs a
>
>>  transition period before  the IR based instrumentation becomes
>
>>  stablized (and can be flipped to the default).  During the transition
>
>>  period, the behavior of 1) does not change, but a cc1 option can be
>
>>  used to turn on IR instrumentation (as proposed by Sean).
>
>
> Just to clarify, users are not allowed to use cc1 options. The cc1 option is 
> purely for us as compiler developers to do integration and testing, put off 
> some discussions for later, etc.
>

yes.

thanks,

David

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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur added a subscriber: xur.
xur added a comment.

OK. Now I understand what you meant. I was trying to avoid driver changes.
It seems you are saying I can change the driver to support cc1 options more
cleanly. This can be done.

Thanks,

Rong


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

OK. Now I understand what you meant. I was trying to avoid driver changes.
It seems you are saying I can change the driver to support cc1 options more
cleanly. This can be done.

Thanks,

Rong
On Jan 25, 2016 12:43, "Sean Silva"  wrote:

> silvas added inline comments.
>
> 
> Comment at: lib/Frontend/CompilerInvocation.cpp:487
> @@ +486,3 @@
> +  // Opts.ProfileInstrGenerate will be used for Clang instrumentation
> only.
> +  if (!Opts.ProfileIRInstr)
> +Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate)
> ||
> 
> davidxl wrote:
> > xur wrote:
> > > silvas wrote:
> > > > I don't like this. It breaks the 1:1 mapping of flags to codegen
> options. Like Chad said, this sort of complexity should be kept in the
> driver.
> > > >
> > > > Some refactoring may be needed to cleanly integrate the new IR
> instrumentation.
> > > hmm. I don't think there is 1:1 mapping b/w flags and codegen options,
> because -fprofile-instr-generate is shared by IR and FE instrumentation.
> > If you rename the FE PGO CodeGen opt name as proposed, it will be
> clearer and less confusing.
> > hmm. I don't think there is 1:1 mapping b/w flags and codegen options,
> because -fprofile-instr-generate is shared by IR and FE instrumentation.
>
> Maybe at the driver level (that discussion has yet to conclude), but at
> cc1 level options should be factored to be clean and orthogonal. Like Chad
> said, the complexity of verifying options that are mutually exclusive or
> that must be present with other options should be done in the driver. cc1
> is a private interface
>
>
>
> http://reviews.llvm.org/D15829
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added inline comments.


Comment at: lib/Frontend/CompilerInvocation.cpp:487
@@ +486,3 @@
+  // Opts.ProfileInstrGenerate will be used for Clang instrumentation only.
+  if (!Opts.ProfileIRInstr)
+Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||

davidxl wrote:
> xur wrote:
> > silvas wrote:
> > > I don't like this. It breaks the 1:1 mapping of flags to codegen options. 
> > > Like Chad said, this sort of complexity should be kept in the driver.
> > > 
> > > Some refactoring may be needed to cleanly integrate the new IR 
> > > instrumentation.
> > hmm. I don't think there is 1:1 mapping b/w flags and codegen options, 
> > because -fprofile-instr-generate is shared by IR and FE instrumentation.
> If you rename the FE PGO CodeGen opt name as proposed, it will be clearer and 
> less confusing.
> hmm. I don't think there is 1:1 mapping b/w flags and codegen options, 
> because -fprofile-instr-generate is shared by IR and FE instrumentation.

Maybe at the driver level (that discussion has yet to conclude), but at cc1 
level options should be factored to be clean and orthogonal. Like Chad said, 
the complexity of verifying options that are mutually exclusive or that must be 
present with other options should be done in the driver. cc1 is a private 
interface



http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

On Mon, Jan 25, 2016 at 12:52 PM, Rong Xu  wrote:

> OK. Now I understand what you meant. I was trying to avoid driver changes.
> It seems you are saying I can change the driver to support cc1 options more
> cleanly. This can be done.
>
Great! Sorry for the confusion.

-- Sean Silva


> Thanks,
>
> Rong
> On Jan 25, 2016 12:43, "Sean Silva"  wrote:
>
>> silvas added inline comments.
>>
>> 
>> Comment at: lib/Frontend/CompilerInvocation.cpp:487
>> @@ +486,3 @@
>> +  // Opts.ProfileInstrGenerate will be used for Clang instrumentation
>> only.
>> +  if (!Opts.ProfileIRInstr)
>> +Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate)
>> ||
>> 
>> davidxl wrote:
>> > xur wrote:
>> > > silvas wrote:
>> > > > I don't like this. It breaks the 1:1 mapping of flags to codegen
>> options. Like Chad said, this sort of complexity should be kept in the
>> driver.
>> > > >
>> > > > Some refactoring may be needed to cleanly integrate the new IR
>> instrumentation.
>> > > hmm. I don't think there is 1:1 mapping b/w flags and codegen
>> options, because -fprofile-instr-generate is shared by IR and FE
>> instrumentation.
>> > If you rename the FE PGO CodeGen opt name as proposed, it will be
>> clearer and less confusing.
>> > hmm. I don't think there is 1:1 mapping b/w flags and codegen options,
>> because -fprofile-instr-generate is shared by IR and FE instrumentation.
>>
>> Maybe at the driver level (that discussion has yet to conclude), but at
>> cc1 level options should be factored to be clean and orthogonal. Like Chad
>> said, the complexity of verifying options that are mutually exclusive or
>> that must be present with other options should be done in the driver. cc1
>> is a private interface
>>
>>
>>
>> http://reviews.llvm.org/D15829
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

On Mon, Jan 25, 2016 at 11:00 AM, Xinliang David Li 
wrote:

> On Fri, Jan 22, 2016 at 1:43 PM, Sean Silva  wrote:
> > silvas added a comment.
> >
> > In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
> >
> >> For the longer term, one possible solution is to make FE based
> >>  instrumentation only used for coverage testing which can be turned on
> >>  with -fcoverage-mapping option (currently, -fcoverage-mapping can not
> >>  be used alone and must be used together with
> >>  -fprofile-instr-generate). To summarize:
> >>
> >> A. Current behavior:
> >>
> >>  ---
> >>
> >> 1. -fprofile-instr-generate turns on FE based instrumentation
> >> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based
> instrumentation and coverage mapping data generation.
> >> 3. -fprofile-instr-use=<..> assumes profile data from FE
> instrumentation.
> >>
> >> B. Proposed new behavior:
> >>
> >>  
> >>
> >> 1. -fprofile-instr-generate turns on IR late instrumentation
> >> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
> >> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler
> warning
> >> 4. -fprofile-instr-use=<> will automatically determine how to use the
> profile data.
> >
> >
> > Very good observation that we can determine FE or IR automatically based
> on the input profdata. That simplifies things.
> >
> >> B.2) above can be done today for improved usability.
> >
> >
> > I don't see how this improves usability. In fact, it is confusing
> because it hijacks an existing option.
> >
> > Also B.3 causes existing user builds to emit a warning, which is
> annoying.
>
> Since it is pointless to specify -fcoverage-mapping option alone, why
> not do not automatically? Yes it means some behavior change (in a good
> way) and  some annoying warnings initially, but those are trivially
> fixable by the user.
>

"trivially fixable" needs to be weighed against the number of times it
needs to be fixed across the user base.

Anyway, emitting a warning for this is clearly a separate discussion
(separate even from the discussion of whether to accept just
`-fcoverage-mapping` without `-fprofile-instr-generate`), so let's not get
hung up on it.


>
>
> >
> > I would propose the following modification of B:
> >
> > C.:
> >
> > 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves
> exactly as before, except that it uses IR instrumentation)
> > 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend
> instrumentation and coverage. (i.e. behaves exactly as before)
>
> I am fine with this -- as long as the user does not need to bear the
> burden of understanding where and how the instrumentation is done.
>

Agreed.


>
> > 3. -fprofile-instr-use=<> automatically determines which method to use
> >
> > All existing user workflows continue to work, except for workflows that
> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they
> have checked-in to version control and represents some special workload)
> with the profile data from new binaries.
> >
> > Concretely, imagine the following workflow:
> >
> >   clang -fprofile-instr-generate foo.c -o foo
> >   ./foo
> >   llvm-profdata merge default.profraw -o new.profdata
> >   llvm-profdata merge new.profdata
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata
> -o foo.profdata
> >   clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
> >
> > I think this is a reasonable breakage. We would need to add a note in
> the release notes. Unfortunately this is not expected breakage if we claim
> to have forward compatibility for profdata (which IIRC Apple requires;
> @bogner?). But I think this case will be rare and exceptional enough that
> we can tolerate it:
> >
>
> The old profile data has to out live the program source versions which
> usually change fast. In reality, I don't expect profile data to live
> too long.
>

I don't either, but it's not unthinkable. E.g. a user may have the sources
of a dependency checked-in, and they only update the profdata for those
sources when they update the dependency.

But overall, I think that release notes mentioning the workaround for this
situation is sufficient.

-- Sean Silva


>
> > - a simple immediate workaround is to specify `-fcoverage-mapping`
> (which also adds some extra stuff, but works around the issue)
>
> This is a reasonable workaround
>
> > - Presumably
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata
> is regenerated with some frequency which is more frequent than upgrading
> the compiler, and so it is likely reasonable to regenerate it alongside a
> compiler upgrade, using the workaround until then.
> >
> >
> >
> >> B.1) needs a
> >
> >>  transition period before  the IR based instrumentation becomes
> >
> >>  stablized (and can be flipped to the default).  During the transition
> >
> >>  period, 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added inline comments.


Comment at: lib/CodeGen/BackendUtil.cpp:440
@@ +439,3 @@
+  if (CodeGenOpts.ProfileIRInstr) {
+// Should not have ProfileInstrGenerate set -- it is for clang
+// instrumentation only.

xur wrote:
> silvas wrote:
> > Then change the existing references in clang. Please try to cleanly 
> > integrate the new functionality, not "sneak it in" with the smallest number 
> > of lines changed (and creating technical debt for future maintainers).
> The integration is actually clean to me: For CodeGenOpt: ProfileInstrGenerate 
> is for Clang instrumentation and ProfileIRInstr is for IR instrumentation. 
> They need to mutually exclusive. 
> 
> Maybe I need to modify the name and description for  ProfileInstrGenerate to 
> reflect the change.
yes, you can change ProfileInstrGenerate to something like ClangProfInstrGen as 
a NFC first.


Comment at: lib/Frontend/CompilerInvocation.cpp:487
@@ +486,3 @@
+  // Opts.ProfileInstrGenerate will be used for Clang instrumentation only.
+  if (!Opts.ProfileIRInstr)
+Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||

xur wrote:
> silvas wrote:
> > I don't like this. It breaks the 1:1 mapping of flags to codegen options. 
> > Like Chad said, this sort of complexity should be kept in the driver.
> > 
> > Some refactoring may be needed to cleanly integrate the new IR 
> > instrumentation.
> hmm. I don't think there is 1:1 mapping b/w flags and codegen options, 
> because -fprofile-instr-generate is shared by IR and FE instrumentation.
If you rename the FE PGO CodeGen opt name as proposed, it will be clearer and 
less confusing.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

Here is the new patch. Changes from previous patch are:
(1) rename codegen option ProfileInstrGenerate to ClangProfInstrGen to avoid 
the name confusion (Davidxl). This option is for clang instrumentation only.
(2) improve the test suggested by Sean.
(3) move the logic of checking if argument ProfileIRInstr needs to be added to 
Tools.C (suggested by Chad and Sean). This still does not look very clean -- I 
need to manually expand the -Xclang option for this. The difficulty here is 
that we have shared driver level arguments (-fprofile-generate and 
-fprofile-use etc) and there is no cc1 option for clang instrumentation. Once 
we have driver level option for IR level instrumentation, the code can be 
improved.


http://reviews.llvm.org/D15829

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGStmt.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenPGO.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/pgotest.profraw
  test/CodeGen/pgo-instrumentation.c

Index: test/CodeGen/pgo-instrumentation.c
===
--- /dev/null
+++ test/CodeGen/pgo-instrumentation.c
@@ -0,0 +1,23 @@
+// Test if PGO instrumentation and use pass are invoked.
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-V1
+// CHECK-PGOGENPASS-INVOKED-V1: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr -fprofile-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-V2
+// CHECK-PGOGENPASS-INVOKED-V2: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr -fprofile-instr-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-V1
+// CHECK-PGOUSEPASS-INVOKED-V1: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-V2
+// CHECK-PGOUSEPASS-INVOKED-V2: PGOInstrumentationUsePass
+//
+// Ensure cc1 option -fprofile-ir-instr is null operation without existing pgo options.
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOPASS-INVOKED
+// CHECK-PGOPASS-INVOKED-NOT: PGOInstrumentation{Gen|USE}Pass
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -477,8 +477,13 @@
   Opts.DisableIntegratedAS = Args.hasArg(OPT_fno_integrated_as);
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
-  Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
-  Args.hasArg(OPT_fprofile_instr_generate_EQ);
+  if (Args.hasArg(OPT_fprofile_ir_instr))
+// Set ProfileIRInstr when there are profile generate or use options.
+Opts.ProfileIRInstr = true;
+  else
+// Opts.ClangProfInstrGen will be used for Clang instrumentation only.
+Opts.ClangProfInstrGen = Args.hasArg(OPT_fprofile_instr_generate) ||
+ Args.hasArg(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileOutput = Args.getLastArgValue(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileInput = Args.getLastArgValue(OPT_fprofile_instr_use_EQ);
   Opts.CoverageMapping =
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -5514,7 +5514,20 @@
 
   // Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option
   // parser.
-  Args.AddAllArgValues(CmdArgs, options::OPT_Xclang);
+  //Args.AddAllArgValues(CmdArgs, options::OPT_Xclang);
+  for (const Arg *A : Args.filtered(options::OPT_Xclang)) {
+A->claim();
+if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
+(Args.hasArg(options::OPT_fprofile_instr_generate) ||
+ Args.hasArg(options::OPT_fprofile_instr_generate_EQ) ||
+ Args.hasArg(options::OPT_fprofile_generate) ||
+ Args.hasArg(options::OPT_fprofile_generate_EQ) ||
+ 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added inline comments.


Comment at: lib/Driver/Tools.cpp:3279
@@ -3278,1 +3278,3 @@
+
+  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
 }

xur wrote:
> mcrosier wrote:
> > I don't think AddAllArgs is what you really want.  What if the user 
> > specifies the option twice?  Do we really want to pass the flag from the 
> > driver to the front-end twice?  Also, should we warn if the option is 
> > passed twice?
> > 
> > I also think some of the logic in CompilerInvocation should land here...  
> > see comments below.
> Thanks for pointing thi out. What about add a guard, like:
> -  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
> +  if (Args.hasArg(options::OPT_fprofile_ir_instr))
> +Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
> 
> But looking at it again, I think i need to remove this stmt in the most 
> recently patch as OPT_profile_ir_instr is now a CC1 option. it will not be 
> seen here, right?
> 
> 
> But looking at it again, I think i need to remove this stmt in the most 
> recently patch as OPT_profile_ir_instr is now a CC1 option. it will not be 
> seen here, right?

Yes. The patch should make no changes to lib/Driver and should require no tests 
in test/Driver


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur added inline comments.


Comment at: lib/Driver/Tools.cpp:3279
@@ -3278,1 +3278,3 @@
+
+  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
 }

mcrosier wrote:
> I don't think AddAllArgs is what you really want.  What if the user specifies 
> the option twice?  Do we really want to pass the flag from the driver to the 
> front-end twice?  Also, should we warn if the option is passed twice?
> 
> I also think some of the logic in CompilerInvocation should land here...  see 
> comments below.
Thanks for pointing thi out. What about add a guard, like:
-  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
+  if (Args.hasArg(options::OPT_fprofile_ir_instr))
+Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);

But looking at it again, I think i need to remove this stmt in the most 
recently patch as OPT_profile_ir_instr is now a CC1 option. it will not be seen 
here, right?




Comment at: lib/Frontend/CompilerInvocation.cpp:482
@@ +481,3 @@
+  Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr) &&
+(Args.hasArg(OPT_fprofile_instr_generate) ||
+ Args.hasArg(OPT_fprofile_instr_generate_EQ) ||

mcrosier wrote:
> IIRC, the general practice is to put this type of logic in the driver.  
> Specifically, in Driver.cpp include something like
> 
> if (Args.hasArg(OPT_fprofile_ir_instr) &&
> (Args.hasArg(OPT_fprofile_instr_generate) ||
>  Args.hasArg(OPT_fprofile_instr_generate_EQ) ||
>  Args.hasArg(OPT_fprofile_instr_use_EQ)))
>   // Add -fprofile_ir_instr flag
> 
> 
In the most recent patch (Suggested by Sean as a temporary measure to speed up 
the code review)  where -fprofiel-ir-instr becomes a CC1 only option, I have to 
do it here. Driver won't see the OPT_fprofile_ir_instr. 

But I'll remember you point when this becomes a Driver option in the next step. 
Also note that the driver could convert "-fprofile-genearte" to 
"-fprofiel-instr-generate" and also for "-fprofile-use".  So the condition will 
be longer if we move to driver.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added a comment.

In http://reviews.llvm.org/D15829#333902, @davidxl wrote:

> For the longer term, one possible solution is to make FE based
>  instrumentation only used for coverage testing which can be turned on
>  with -fcoverage-mapping option (currently, -fcoverage-mapping can not
>  be used alone and must be used together with
>  -fprofile-instr-generate). To summarize:
>
> A. Current behavior:
>
>  ---
>
> 1. -fprofile-instr-generate turns on FE based instrumentation
> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
> instrumentation and coverage mapping data generation.
> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>
> B. Proposed new behavior:
>
>  
>
> 1. -fprofile-instr-generate turns on IR late instrumentation
> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
> 4. -fprofile-instr-use=<> will automatically determine how to use the profile 
> data.


Very good observation that we can determine FE or IR automatically based on the 
input profdata. That simplifies things.

> B.2) above can be done today for improved usability.


I don't see how this improves usability. In fact, it is confusing because it 
hijacks an existing option.

Also B.3 causes existing user builds to emit a warning, which is annoying.

I would propose the following modification of B:

C.:

1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
exactly as before, except that it uses IR instrumentation)
2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
instrumentation and coverage. (i.e. behaves exactly as before)
3. -fprofile-instr-use=<> automatically determines which method to use

All existing user workflows continue to work, except for workflows that attempt 
to `llvm-profdata merge` some old frontend profile data (e.g. they have 
checked-in to version control and represents some special workload) with the 
profile data from new binaries.

Concretely, imagine the following workflow:

  clang -fprofile-instr-generate foo.c -o foo
  ./foo
  llvm-profdata merge default.profraw -o new.profdata
  llvm-profdata merge new.profdata 
/versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata -o 
foo.profdata
  clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo

I think this is a reasonable breakage. We would need to add a note in the 
release notes. Unfortunately this is not expected breakage if we claim to have 
forward compatibility for profdata (which IIRC Apple requires; @bogner?). But I 
think this case will be rare and exceptional enough that we can tolerate it:

- a simple immediate workaround is to specify `-fcoverage-mapping` (which also 
adds some extra stuff, but works around the issue)
- Presumably 
/versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata is 
regenerated with some frequency which is more frequent than upgrading the 
compiler, and so it is likely reasonable to regenerate it alongside a compiler 
upgrade, using the workaround until then.



> B.1) needs a

>  transition period before  the IR based instrumentation becomes

>  stablized (and can be flipped to the default).  During the transition

>  period, the behavior of 1) does not change, but a cc1 option can be

>  used to turn on IR instrumentation (as proposed by Sean).


Just to clarify, users are not allowed to use cc1 options. The cc1 option is 
purely for us as compiler developers to do integration and testing, put off 
some discussions for later, etc.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added a comment.

For the longer term, one possible solution is to make FE based
instrumentation only used for coverage testing which can be turned on
with -fcoverage-mapping option (currently, -fcoverage-mapping can not
be used alone and must be used together with
-fprofile-instr-generate). To summarize:

A. Current behavior:


1. -fprofile-instr-generate turns on FE based instrumentation
2. -fprofile-instr-generate -fcoverage-mapping turns on FE based

instrumentation and coverage mapping data generation.

3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.



B. Proposed new behavior:
-

1. -fprofile-instr-generate turns on IR late instrumentation
2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
4. -fprofile-instr-use=<> will automatically determine how to use the

profile data.

B.2) above can be done today for improved usability. B.1) needs a
transition period before  the IR based instrumentation becomes
stablized (and can be flipped to the default).  During the transition
period, the behavior of 1) does not change, but a cc1 option can be
used to turn on IR instrumentation (as proposed by Sean).

In the real longer term, I think IR based instrumentation can also be
used for coverage testing too (by disabling some pre-optimizations and
pre-inlining needed for PGO purpose) -- but this is a different topic
to be discussed.

thanks,

David


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

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

This new patches integrates Sean review comments:
(1) make -fprofile-ir-instr a cc1 option instead of driver option.
(2) add one cc1 option test, and test the pass is indeed invoked.
(3) remove the driver test (runtime library).
(4) fix a comment.

Thanks,

-Rong


http://reviews.llvm.org/D15829

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/pgo-instrumentation.c

Index: test/CodeGen/pgo-instrumentation.c
===
--- /dev/null
+++ test/CodeGen/pgo-instrumentation.c
@@ -0,0 +1,9 @@
+// Test PGO instrumentation.
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr -fprofile-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PASS
+// CHECK-PASS: PGOInstrumentationGenPass
+//
+// Ensure cc1 option -fprofile-ir-instr is null operation without existing pgo options.
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PASS2
+// CHECK-PASS2-NOT: PGOInstrumentationGenPass
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -477,8 +477,16 @@
   Opts.DisableIntegratedAS = Args.hasArg(OPT_fno_integrated_as);
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
-  Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
-  Args.hasArg(OPT_fprofile_instr_generate_EQ);
+  // Only set ProfileIRInstr when there are profile generate or use options.
+  Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr) &&
+(Args.hasArg(OPT_fprofile_instr_generate) ||
+ Args.hasArg(OPT_fprofile_instr_generate_EQ) ||
+ Args.hasArg(OPT_fprofile_instr_use_EQ));
+  // Set Opts.ProfileInstrGenerate when Opts.ProfileIRInstr is false.
+  // Opts.ProfileInstrGenerate will be used for Clang instrumentation only.
+  if (!Opts.ProfileIRInstr)
+Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
+Args.hasArg(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileOutput = Args.getLastArgValue(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileInput = Args.getLastArgValue(OPT_fprofile_instr_use_EQ);
   Opts.CoverageMapping =
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -3275,6 +3275,8 @@
   CmdArgs.push_back(Args.MakeArgString(CoverageFilename));
 }
   }
+
+  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
 }
 
 static void addPS4ProfileRTArgs(const ToolChain , const ArgList ,
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -147,7 +147,7 @@
   if (C.getLangOpts().ObjC1)
 ObjCData = new ObjCEntrypoints();
 
-  if (!CodeGenOpts.InstrProfileInput.empty()) {
+  if (!CodeGenOpts.ProfileIRInstr && !CodeGenOpts.InstrProfileInput.empty()) {
 auto ReaderOrErr =
 llvm::IndexedInstrProfReader::create(CodeGenOpts.InstrProfileInput);
 if (std::error_code EC = ReaderOrErr.getError()) {
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -436,6 +436,18 @@
 MPM->add(createInstrProfilingPass(Options));
   }
 
+  if (CodeGenOpts.ProfileIRInstr) {
+// Should not have ProfileInstrGenerate set -- it is for clang
+// instrumentation only.
+assert (!CodeGenOpts.ProfileInstrGenerate);
+if (!CodeGenOpts.InstrProfileInput.empty())
+  PMBuilder.PGOInstrUse = CodeGenOpts.InstrProfileInput;
+else if (!CodeGenOpts.InstrProfileOutput.empty())
+  PMBuilder.PGOInstrGen = CodeGenOpts.InstrProfileOutput;
+else
+  PMBuilder.PGOInstrGen = "default.profraw";
+  }
+
   if (!CodeGenOpts.SampleProfileFile.empty())
 MPM->add(createSampleProfileLoaderPass(CodeGenOpts.SampleProfileFile));
 
Index: include/clang/Frontend/CodeGenOptions.def
===
--- include/clang/Frontend/CodeGenOptions.def
+++ include/clang/Frontend/CodeGenOptions.def
@@ -105,6 +105,7 @@
 
 CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate
 ///< execution counts to use with PGO.
+CODEGENOPT(ProfileIRInstr, 1, 0) ///< IR level PGO instrumentation and use.
 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Chad Rosier via cfe-commits]

mcrosier added a subscriber: mcrosier.
mcrosier added a comment.

Would it make sense to include an additional test (in test/Driver) that shows 
the -fprofile-ir-instr option being passed from the driver to the frontend? 
Such a test case would land in clang_f_opt.c, which has many examples.



Comment at: lib/Driver/Tools.cpp:3279
@@ -3278,1 +3278,3 @@
+
+  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
 }

I don't think AddAllArgs is what you really want.  What if the user specifies 
the option twice?  Do we really want to pass the flag from the driver to the 
front-end twice?  Also, should we warn if the option is passed twice?

I also think some of the logic in CompilerInvocation should land here...  see 
comments below.


Comment at: lib/Frontend/CompilerInvocation.cpp:482
@@ +481,3 @@
+  Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr) &&
+(Args.hasArg(OPT_fprofile_instr_generate) ||
+ Args.hasArg(OPT_fprofile_instr_generate_EQ) ||

IIRC, the general practice is to put this type of logic in the driver.  
Specifically, in Driver.cpp include something like

if (Args.hasArg(OPT_fprofile_ir_instr) &&
(Args.hasArg(OPT_fprofile_instr_generate) ||
 Args.hasArg(OPT_fprofile_instr_generate_EQ) ||
 Args.hasArg(OPT_fprofile_instr_use_EQ)))
  // Add -fprofile_ir_instr flag




http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

On Thu, Jan 21, 2016 at 7:32 PM, Sean Silva  wrote:
> silvas added a comment.
>
> @slingn and I had a discussion offline about the potential names and came up 
> with some ideas, but none is a clear winner.
>
> Overall, my feeling is that from a user's perspective, the frontend stuff is 
> probably best referred to as "coverage-based". It's not as clear for the 
> IR-level stuff, but referring to it as the "pgo focused" or "optimization 
> focused" instrumentation might be a way to describe it. E.g. perhaps 
> `-fprofile-instr-method={coverage,optimization}`.
>

Agreed. Frontend instrumentation has a less chance of the mismatch
caused by compiler change. IR-level instrumentation is more for the
PGO optimization. Speed and simplicity are the focus -- that is also
the main reason that we don't split the BB for no-return-calls.

> For now, can we make this a CC1-only option? Then we don't have to hold up 
> the patch review on the driver stuff. Once we have fully integrated the 
> IR-level instrumentation, we can revisit exposing the user-visible name. (as 
> compiler developers, we can of course use the flag freely for 
> integration/testing).

I can do this. I just need to move the option define from Opertion.td
to CC1Options.td. The user would need to use '-Xclang
-fprofile-ir-instr' to invoke.

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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added inline comments.


Comment at: lib/CodeGen/BackendUtil.cpp:440
@@ +439,3 @@
+  if (CodeGenOpts.ProfileIRInstr) {
+// Should not have ProfileInstrGenerate set -- it is for clang
+// instrumentation only.

Then change the existing references in clang. Please try to cleanly integrate 
the new functionality, not "sneak it in" with the smallest number of lines 
changed (and creating technical debt for future maintainers).


Comment at: lib/Frontend/CompilerInvocation.cpp:482
@@ +481,3 @@
+  Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr) &&
+(Args.hasArg(OPT_fprofile_instr_generate) ||
+ Args.hasArg(OPT_fprofile_instr_generate_EQ) ||

xur wrote:
> mcrosier wrote:
> > IIRC, the general practice is to put this type of logic in the driver.  
> > Specifically, in Driver.cpp include something like
> > 
> > if (Args.hasArg(OPT_fprofile_ir_instr) &&
> > (Args.hasArg(OPT_fprofile_instr_generate) ||
> >  Args.hasArg(OPT_fprofile_instr_generate_EQ) ||
> >  Args.hasArg(OPT_fprofile_instr_use_EQ)))
> >   // Add -fprofile_ir_instr flag
> > 
> > 
> In the most recent patch (Suggested by Sean as a temporary measure to speed 
> up the code review)  where -fprofiel-ir-instr becomes a CC1 only option, I 
> have to do it here. Driver won't see the OPT_fprofile_ir_instr. 
> 
> But I'll remember you point when this becomes a Driver option in the next 
> step. Also note that the driver could convert "-fprofile-genearte" to 
> "-fprofiel-instr-generate" and also for "-fprofile-use".  So the condition 
> will be longer if we move to driver.
Chad makes a good point though. This should be just `Opts.ProfileIRInstr = 
Args.hasArg(OPT_fprofile_ir_instr);` because we can assume that for cc11 this 
option is not used except in conjunction with `-fprofile-instr-{generate,use}`


Comment at: lib/Frontend/CompilerInvocation.cpp:487
@@ +486,3 @@
+  // Opts.ProfileInstrGenerate will be used for Clang instrumentation only.
+  if (!Opts.ProfileIRInstr)
+Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||

I don't like this. It breaks the 1:1 mapping of flags to codegen options. Like 
Chad said, this sort of complexity should be kept in the driver.

Some refactoring may be needed to cleanly integrate the new IR instrumentation.


Comment at: test/CodeGen/pgo-instrumentation.c:9
@@ +8,2 @@
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr %s -mllvm 
-debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PASS2
+// CHECK-PASS2-NOT: PGOInstrumentationGenPass

Please test both Gen and Use phases.

Also, please use more descriptive CHECK names.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur added a comment.

Ping.

Passmanagerbuilder change has been committed. Could you take a look at this 
command line option patch?

Thanks,

-Rong


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added a comment.

This needs tests showing that the IR gen/use passes get run. Maybe use 
-debug-pass=Structure like test/CodeGen/thinlto_backend.c?

My biggest concern is the naming and user visible parts. I can't come up with 
anything better than `-fprofile-ir-instr` TBH. Overall, from a user's 
perspective, this is really just "perform instrumentation in an alternate way", 
since "IR" vs "frontend" is really not meaningful for them (there isn't really 
any meaningful way for use to communicate anything to them about what they 
should expect the flag to do differently as user-visible behavior). 
`-fprofile-alternate-instr` doesn't sound any better really.

Any ideas?

Once we expose this as a driver option though we must remain compatible, so it 
is best to think for a moment about the naming.



Comment at: include/clang/Driver/Options.td:457
@@ -455,1 +456,3 @@
+Flags<[CC1Option]>,
+HelpText<"Use IR level instrumentation rather the FE implementation">;
 def fcoverage_mapping : Flag<["-"], "fcoverage-mapping">,

This doesn't seem like useful help text for a user.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added a comment.

@slingn and I had a discussion offline about the potential names and came up 
with some ideas, but none is a clear winner.

Overall, my feeling is that from a user's perspective, the frontend stuff is 
probably best referred to as "coverage-based". It's not as clear for the 
IR-level stuff, but referring to it as the "pgo focused" or "optimization 
focused" instrumentation might be a way to describe it. E.g. perhaps 
`-fprofile-instr-method={coverage,optimization}`.

For now, can we make this a CC1-only option? Then we don't have to hold up the 
patch review on the driver stuff. Once we have fully integrated the IR-level 
instrumentation, we can revisit exposing the user-visible name. (as compiler 
developers, we can of course use the flag freely for integration/testing).


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added inline comments.


Comment at: include/clang/Frontend/CodeGenOptions.def:108
@@ -107,2 +107,3 @@
 ///< execution counts to use with PGO.
+CODEGENOPT(ProfileIRInstr, 1, 0) ///< IR level code PGO instrumentation and 
use.
 CODEGENOPT(CoverageMapping , 1, 0) ///< Generate coverage mapping regions to

tiny nit: the word "code" seems redundant here.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Sean Silva via cfe-commits]

silvas added a comment.

@bogner btw, did you say that at Apple you guys have a requirement of 
supporting existing profdata? I.e. users can pass older profdata to a newer 
compiler?

Realistically, it would be nice if our PGO offering defaulted to the IR stuff 
(since it seems like it is going to be the focus of optimization work and so is 
likely to be our best-performing offering). But if we need to support profdata 
across versions of clang, that puts us in a thorny situation because suddenly 
`clang -c foo.c 
-fprofile-instr-use=old-profdata-from-frontend-instrumentation.profdata` has a 
default meaning equivalent to `clang -c foo.c -fir-level-instrumentation 
-fprofile-instr-use=old-profdata-from-frontend-instrumentation.profdata` 
(setting aside the flag name issue), which is a case we want to error on due to 
mismatch between frontend and IR-level instrumentation.


http://reviews.llvm.org/D15829



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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[David Li via cfe-commits]

davidxl added a comment.

Should add a test case in test/Driver/instrprof-ld.c.



Comment at: lib/CodeGen/BackendUtil.cpp:435
@@ +434,3 @@
+  if (CodeGenOpts.ProfileIRInstr) {
+assert (!CodeGenOpts.ProfileInstrGenerate);
+if (!CodeGenOpts.InstrProfileOutput.empty())

What is this asserting about?


Comment at: lib/Frontend/CompilerInvocation.cpp:453
@@ +452,3 @@
+  Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr);
+  if (!Opts.ProfileIRInstr)
+Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||

Add a comment here.


http://reviews.llvm.org/D15829



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

[PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur created this revision.
xur added reviewers: davidxl, silvas, bogner.
xur added a subscriber: cfe-commits.

This patch introduce a new toggle option -fprofile-ir-instr that enables IR 
level instrumentation. It needs to be used in combination with current PGO 
options. Without an existing PGO options, this option alone is a null operation.
 
This patch depends on llvm patch http://reviews.llvm.org/D15828 where we 
introduce a pair of passmanagerbuilder options: 
 -profile-generate=
 -profile-use=
Note that the profile specified in driver level options 
(-fprofile-instr-generate, -fprofile-instr-use, -fprofile-generate, and 
-profile-use) overrides the profile in the passmanagerbuilder option.


http://reviews.llvm.org/D15829

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp

Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -449,8 +449,10 @@
   Opts.DisableIntegratedAS = Args.hasArg(OPT_fno_integrated_as);
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
-  Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
-  Args.hasArg(OPT_fprofile_instr_generate_EQ);
+  Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr);
+  if (!Opts.ProfileIRInstr)
+Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
+Args.hasArg(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileOutput = 
Args.getLastArgValue(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileInput = Args.getLastArgValue(OPT_fprofile_instr_use_EQ);
   Opts.CoverageMapping =
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -3223,6 +3223,8 @@
   CmdArgs.push_back(Args.MakeArgString(CoverageFilename));
 }
   }
+
+  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
 }
 
 static void addPS4ProfileRTArgs(const ToolChain , const ArgList ,
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -147,7 +147,7 @@
   if (C.getLangOpts().ObjC1)
 ObjCData = new ObjCEntrypoints();
 
-  if (!CodeGenOpts.InstrProfileInput.empty()) {
+  if (!CodeGenOpts.ProfileIRInstr && !CodeGenOpts.InstrProfileInput.empty()) {
 auto ReaderOrErr =
 llvm::IndexedInstrProfReader::create(CodeGenOpts.InstrProfileInput);
 if (std::error_code EC = ReaderOrErr.getError()) {
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -431,6 +431,14 @@
 MPM->add(createInstrProfilingPass(Options));
   }
 
+  if (CodeGenOpts.ProfileIRInstr) {
+assert (!CodeGenOpts.ProfileInstrGenerate);
+if (!CodeGenOpts.InstrProfileOutput.empty())
+  PMBuilder.PGOInstrGen = CodeGenOpts.InstrProfileOutput;
+if (!CodeGenOpts.InstrProfileInput.empty())
+  PMBuilder.PGOInstrUse = CodeGenOpts.InstrProfileInput;
+  }
+
   if (!CodeGenOpts.SampleProfileFile.empty())
 MPM->add(createSampleProfileLoaderPass(CodeGenOpts.SampleProfileFile));
 
Index: include/clang/Frontend/CodeGenOptions.def
===
--- include/clang/Frontend/CodeGenOptions.def
+++ include/clang/Frontend/CodeGenOptions.def
@@ -105,6 +105,7 @@
 
 CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate
 ///< execution counts to use with PGO.
+CODEGENOPT(ProfileIRInstr, 1, 0) ///< IR level code PGO instrumentation and 
use.
 CODEGENOPT(CoverageMapping , 1, 0) ///< Generate coverage mapping regions to
///< enable code coverage analysis.
 CODEGENOPT(DumpCoverageMapping , 1, 0) ///< Dump the generated coverage mapping
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -452,6 +452,9 @@
 def fprofile_instr_use_EQ : Joined<["-"], "fprofile-instr-use=">,
 Group, Flags<[CC1Option]>,
 HelpText<"Use instrumentation data for profile-guided optimization">;
+def fprofile_ir_instr : Flag<["-"], "fprofile-ir-instr">, Group,
+Flags<[CC1Option]>,
+HelpText<"Use IR level instrumentation rather the FE implementation">;
 def fcoverage_mapping : Flag<["-"], "fcoverage-mapping">,
 Group, Flags<[CC1Option]>,
 HelpText<"Generate coverage mapping to enable code coverage analysis">;


Index: lib/Frontend/CompilerInvocation.cpp