[PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation
Sean Silva via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 22 14:03:33 PST 2016
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
More information about the cfe-commits
mailing list