[PATCH] [Profile] compiler-rt support for setting profile output from command line
Teresa Johnson
tejohnson at google.com
Tue Apr 28 10:41:31 PDT 2015
On Tue, Apr 28, 2015 at 9:56 AM, Justin Bogner <mail at justinbogner.com> wrote:
> Teresa Johnson <tejohnson at google.com> writes:
>> This change is the first of 3 patches to add support for specifying
>> the profile output from the command line via -fprofile-instr-generate=<path>,
>> where the specified output path/file will be overridden by the
>> LLVM_PROFILE_FILE environment variable.
>>
>> Several changes are made to the runtime to support this:
>> 1) Add a new interface __llvm_profile_set_filename_env_override that will
>> set the profile output filename, but allows the LLVM_PROFILE_FILE to override.
>> This is the interface used by the new option.
>> 2) Refactor the pid-expansion done for LLVM_PROFILE_FILE into a separate
>> routine that can be shared by the various filename setting routines
>> (so that the filename from the option can also use the "%p" syntax).
>> 3) Move the truncation into setFilename, and only truncate if there is a
>> new filename specified (to maintain support for appending to the same
>> profile file in the case of multiple shared objects built with profiling).
>> 4) Move the handling for a NULL filename passed to __llvm_profile_setfilename*
>> into the new setFilenamePossiblyWithPid routine. The handling for a null
>> LLVM_PROFILE_FILE (which should not reset) is done by caller
>> setFilenameFromEnvironment.
>
> Cool. Thanks for working on this! I have a few comments, below.
Thanks for the comments and suggestions. Responses below and updated
patch coming. Teresa
>
>> http://reviews.llvm.org/D9323
>>
>> Files:
>> lib/profile/InstrProfiling.h
>> lib/profile/InstrProfilingFile.c
>> test/profile/instrprof-set-filename-env-override.c
>>
>> EMAIL PREFERENCES
>> http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>> Index: lib/profile/InstrProfiling.h
>> ===================================================================
>> --- lib/profile/InstrProfiling.h
>> +++ lib/profile/InstrProfiling.h
>> @@ -77,6 +77,18 @@
>> */
>> void __llvm_profile_set_filename(const char *Name);
>>
>> +/*!
>> + * \brief Set the filename for writing instrumentation data, unless the
>> + * \c LLVM_PROFILE_FILE environment variable was set.
>> + *
>> + * Unless overridden, sets the filename to be used for subsequent calls to
>> + * \a __llvm_profile_write_file().
>> + *
>> + * \c Name is not copied, so it must remain valid. Passing NULL resets the
>> + * filename logic to the default behaviour.
>
> I'm not sure that passing NULL does what you say it does here. It looks
> to me like it'll end up setting __llvm_profile_CurrentFilename to NULL,
> but __llvm_profile_initialize_file has almost certainly already run, so
> we'll just fail when __llvm_profile_write_file gets called later.
What happens is that __llvm_profile_write_file simply exits without
writing anything (so you end up with a 0-length truncated profile
output file). After thinking about this more, I agree with you that
the correct behavior is to go back to writing to the default
"default.profraw" file. Note however that I have simply preserved the
trunk behavior here. We used to do essentially the same thing (reset
filename to Null, truncate it) from __llvm_profile_set_filename. I
confirmed that with a trunk compiler I get the same behavior when I
call __llvm_profile_set_filename(0) - a truncated 0-length profile
output (to whatever was the CurrentFilename at the time of the
__llvm_profile_set_filename(0) call).
I will change the behavior with the my patch to revert to
default.profraw instead, since that's what the comments in the header
imply should be happening.
>
>> + */
>> +void __llvm_profile_set_filename_env_override(const char *Name);
>
> I find this function name confusing - it sounds to me like it overrides
> the environment, which is the exact opposite of what happens. I don't
> have a particularly good alternative though, as something like
> __llvm_profile_set_filename_unless_env_is_set is a bit verbose.
Ok, I see why this is confusing.
>
> Maybe setDefaultFilename should be renamed to "resetFilenameToDefault",
> and this can be set_default_filename or override_default_filename?
Ok. I like the name change on setDefaultFilename to
resetFilenameToDefault, and will make this one
__llvm_profile_override_default_filename.
>
>> +
>> /*! \brief Register to write instrumentation data to file at exit. */
>> int __llvm_profile_register_write_file_atexit(void);
>>
>> Index: lib/profile/InstrProfilingFile.c
>> ===================================================================
>> --- lib/profile/InstrProfilingFile.c
>> +++ lib/profile/InstrProfilingFile.c
>> @@ -76,14 +76,6 @@
>> __attribute__((weak)) int __llvm_profile_OwnsFilename = 0;
>> __attribute__((weak)) const char *__llvm_profile_CurrentFilename = NULL;
>>
>> -static void setFilename(const char *Filename, int OwnsFilename) {
>> - if (__llvm_profile_OwnsFilename)
>> - free(UNCONST(__llvm_profile_CurrentFilename));
>> -
>> - __llvm_profile_CurrentFilename = Filename;
>> - __llvm_profile_OwnsFilename = OwnsFilename;
>> -}
>> -
>> static void truncateCurrentFile(void) {
>> const char *Filename;
>> FILE *File;
>> @@ -99,19 +91,36 @@
>> fclose(File);
>> }
>>
>> +static void setFilename(const char *Filename, int OwnsFilename) {
>> + /* Check if this is a new filename and therefore needs truncation. */
>> + int NewFile = !__llvm_profile_CurrentFilename ||
>> + (Filename && strcmp(Filename, __llvm_profile_CurrentFilename));
>> + if (__llvm_profile_OwnsFilename)
>> + free(UNCONST(__llvm_profile_CurrentFilename));
>> +
>> + __llvm_profile_CurrentFilename = Filename;
>> + __llvm_profile_OwnsFilename = OwnsFilename;
>> +
>> + /* If not a new file, append to support profiling multiple shared objects. */
>> + if (NewFile)
>> + truncateCurrentFile();
>> +}
>> +
>> static void setDefaultFilename(void) { setFilename("default.profraw", 0); }
>>
>> int getpid(void);
>> -static int setFilenameFromEnvironment(void) {
>> - const char *Filename = getenv("LLVM_PROFILE_FILE");
>> +static int setFilenamePossiblyWithPid(const char *Filename) {
>> #define MAX_PID_SIZE 16
>> char PidChars[MAX_PID_SIZE] = {0};
>> int NumPids = 0, PidLength = 0;
>> char *Allocated;
>> int I, J;
>>
>> - if (!Filename || !Filename[0])
>> - return -1;
>> + /* Reset filename on NULL, except for env var which is checked by caller. */
>> + if (!Filename) {
>> + setFilename(Filename, 0);
>
> I guess this should be setDefaultFilename, as per my comment above.
Ok, will change.
>
>> + return 0;
>> + }
>>
>> /* Check the filename for "%p", which indicates a pid-substitution. */
>> for (I = 0; Filename[I]; ++I)
>> @@ -148,6 +157,15 @@
>> return 0;
>> }
>>
>> +static int setFilenameFromEnvironment(void) {
>> + const char *Filename = getenv("LLVM_PROFILE_FILE");
>> +
>> + if (!Filename || !Filename[0])
>> + return -1;
>> +
>> + return setFilenamePossiblyWithPid(Filename);
>> +}
>> +
>> static void setFilenameAutomatically(void) {
>> if (!setFilenameFromEnvironment())
>> return;
>> @@ -163,13 +181,20 @@
>>
>> /* Detect the filename and truncate. */
>> setFilenameAutomatically();
>> - truncateCurrentFile();
>> }
>>
>> __attribute__((visibility("hidden")))
>> void __llvm_profile_set_filename(const char *Filename) {
>> - setFilename(Filename, 0);
>> - truncateCurrentFile();
>> + setFilenamePossiblyWithPid(Filename);
>> +}
>> +
>> +__attribute__((visibility("hidden")))
>> +void __llvm_profile_set_filename_env_override(const char *Filename) {
>> + /* If the env var is set, skip setting filename from argument. */
>> + const char *Env_Filename = getenv("LLVM_PROFILE_FILE");
>> + if (Env_Filename && Env_Filename[0])
>> + return;
>> + setFilenamePossiblyWithPid(Filename);
>> }
>>
>> __attribute__((visibility("hidden")))
>> Index: test/profile/instrprof-set-filename-env-override.c
>> ===================================================================
>> --- /dev/null
>> +++ test/profile/instrprof-set-filename-env-override.c
>> @@ -0,0 +1,14 @@
>> +// RUN: %clang_profgen -o %t -O3 %s
>> +// RUN: env LLVM_PROFILE_FILE=%t.good.profraw %run %t %t.bad.profraw
>> +// RUN: llvm-profdata merge -o %t.profdata %t.good.profraw
>> +// RUN: %clang_profuse=%t.profdata -o - -S -emit-llvm %s | FileCheck %s
>> +
>> +void __llvm_profile_set_filename_env_override(const char *);
>> +int main(int argc, const char *argv[]) {
>> + // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD1:[0-9]+]]
>> + if (argc < 2)
>> + return 1;
>> + __llvm_profile_set_filename_env_override(argv[1]);
>> + return 0;
>> +}
>> +// CHECK: ![[PD1]] = !{!"branch_weights", i32 1, i32 2}
>
> Please also test that this DTRT when LLVM_PROFILE_FILE isn't set, and
> that calling it with NULL resets to the default correctly.
Ok, will add tests for these.
>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
--
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
More information about the llvm-commits
mailing list