diff mbox series

[RFC] Make 4-stage PGO bootstrap really working

Message ID a1959943-f196-da67-e391-3901ffe68d28@suse.cz
State New
Headers show
Series [RFC] Make 4-stage PGO bootstrap really working | expand

Commit Message

Martin Liška Aug. 30, 2017, 9:45 a.m. UTC 
Hi.

This is follow up which I've just noticed. Main problem we have is that
an instrumented compiler w/ -fprofile-generate (built in $OBJDIR/gcc subfolder)
will generate all *.gcda files in a same dir as *.o files. That's problematic
because we then have *.gcda files spread in 'profile' subfolder (because profile'
compiler builds libgcc) and 'train' subfolder. Eventually in 'feedback' stage
we don't load any *.gcda files :/

Well I really hope we need to set -fprofile-generate=$folder to a $folder. There comes
second problem: all *.gcda files are created as $folder/$aux_base_name.gcda which makes
it useless as we multiple same file names:

$ find . -name expr.c
./libcpp/expr.c
./gcc/expr.c

Thus I suggest patch #0001 that appends full path of current work dir. Patch #0002 sets
a folder for PGO bootstrap. So far so good with a small exception: conftest.gcda files
that trigger -Wcoverage-mismatch. Can we remove these before a stage? Do we do a similar
thing somewhere?

Thoughts?
Thanks,
Martin

Comments

Martin Liška Sept. 14, 2017, 12:20 p.m. UTC | #1 
PING^1

On 08/30/2017 11:45 AM, Martin Liška wrote:
> Hi.
> 
> This is follow up which I've just noticed. Main problem we have is that
> an instrumented compiler w/ -fprofile-generate (built in $OBJDIR/gcc subfolder)
> will generate all *.gcda files in a same dir as *.o files. That's problematic
> because we then have *.gcda files spread in 'profile' subfolder (because profile'
> compiler builds libgcc) and 'train' subfolder. Eventually in 'feedback' stage
> we don't load any *.gcda files :/
> 
> Well I really hope we need to set -fprofile-generate=$folder to a $folder. There comes
> second problem: all *.gcda files are created as $folder/$aux_base_name.gcda which makes
> it useless as we multiple same file names:
> 
> $ find . -name expr.c
> ./libcpp/expr.c
> ./gcc/expr.c
> 
> Thus I suggest patch #0001 that appends full path of current work dir. Patch #0002 sets
> a folder for PGO bootstrap. So far so good with a small exception: conftest.gcda files
> that trigger -Wcoverage-mismatch. Can we remove these before a stage? Do we do a similar
> thing somewhere?
> 
> Thoughts?
> Thanks,
> Martin
>
Martin Liška Oct. 19, 2017, 12:56 p.m. UTC | #2 
PING^2

On 09/14/2017 02:20 PM, Martin Liška wrote:
> PING^1
> 
> On 08/30/2017 11:45 AM, Martin Liška wrote:
>> Hi.
>>
>> This is follow up which I've just noticed. Main problem we have is that
>> an instrumented compiler w/ -fprofile-generate (built in $OBJDIR/gcc subfolder)
>> will generate all *.gcda files in a same dir as *.o files. That's problematic
>> because we then have *.gcda files spread in 'profile' subfolder (because profile'
>> compiler builds libgcc) and 'train' subfolder. Eventually in 'feedback' stage
>> we don't load any *.gcda files :/
>>
>> Well I really hope we need to set -fprofile-generate=$folder to a $folder. There comes
>> second problem: all *.gcda files are created as $folder/$aux_base_name.gcda which makes
>> it useless as we multiple same file names:
>>
>> $ find . -name expr.c
>> ./libcpp/expr.c
>> ./gcc/expr.c
>>
>> Thus I suggest patch #0001 that appends full path of current work dir. Patch #0002 sets
>> a folder for PGO bootstrap. So far so good with a small exception: conftest.gcda files
>> that trigger -Wcoverage-mismatch. Can we remove these before a stage? Do we do a similar
>> thing somewhere?
>>
>> Thoughts?
>> Thanks,
>> Martin
>>
>
Markus Trippelsdorf Oct. 19, 2017, 2:27 p.m. UTC | #3 
On 2017.10.19 at 14:56 +0200, Martin Liška wrote:
> PING^2

> So far so good with a small exception: conftest.gcda files that
> trigger -Wcoverage-mismatch. Can we remove these before a stage? Do we
> do a similar thing somewhere?

I think you should simply remove all these conftest.gcda files before
STAGEfeedback.
Markus Trippelsdorf Oct. 25, 2017, 12:19 p.m. UTC | #4 
On 2017.08.30 at 11:45 +0200, Martin Liška wrote:
> diff --git a/Makefile.in b/Makefile.in
> index 78db0982ba2..16b76906ad0 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -529,13 +529,14 @@ STAGE1_CONFIGURE_FLAGS = --disable-intermodule $(STAGE1_CHECKING) \
>  	  --disable-coverage --enable-languages="$(STAGE1_LANGUAGES)" \
>  	  --disable-build-format-warnings
>  
> -STAGEprofile_CFLAGS = $(STAGE2_CFLAGS) -fprofile-generate
> +profile_folder=`${PWD_COMMAND}`/gcov-profiles/
> +STAGEprofile_CFLAGS = $(STAGE2_CFLAGS) -fprofile-generate=$(profile_folder)
>  STAGEprofile_TFLAGS = $(STAGE2_TFLAGS)
>  
>  STAGEtrain_CFLAGS = $(STAGE3_CFLAGS)
>  STAGEtrain_TFLAGS = $(STAGE3_TFLAGS)
>  
> -STAGEfeedback_CFLAGS = $(STAGE4_CFLAGS) -fprofile-use
> +STAGEfeedback_CFLAGS = $(STAGE4_CFLAGS) -fprofile-use=$(profile_folder) -fdump-ipa-profile

-fdump-ipa-profile looks like a debugging leftover and should be
dropped.

>  STAGEfeedback_TFLAGS = $(STAGE4_TFLAGS)
>  
>  STAGEautoprofile_CFLAGS = $(STAGE2_CFLAGS) -g
> diff --git a/Makefile.tpl b/Makefile.tpl
> index 5fcd7e358d9..129175a579c 100644
> --- a/Makefile.tpl
> +++ b/Makefile.tpl
> @@ -452,13 +452,14 @@ STAGE1_CONFIGURE_FLAGS = --disable-intermodule $(STAGE1_CHECKING) \
>  	  --disable-coverage --enable-languages="$(STAGE1_LANGUAGES)" \
>  	  --disable-build-format-warnings
>  
> -STAGEprofile_CFLAGS = $(STAGE2_CFLAGS) -fprofile-generate
> +profile_folder=`${PWD_COMMAND}`/gcov-profiles/
> +STAGEprofile_CFLAGS = $(STAGE2_CFLAGS) -fprofile-generate=$(profile_folder)
>  STAGEprofile_TFLAGS = $(STAGE2_TFLAGS)
>  
>  STAGEtrain_CFLAGS = $(STAGE3_CFLAGS)
>  STAGEtrain_TFLAGS = $(STAGE3_TFLAGS)
>  
> -STAGEfeedback_CFLAGS = $(STAGE4_CFLAGS) -fprofile-use
> +STAGEfeedback_CFLAGS = $(STAGE4_CFLAGS) -fprofile-use=$(profile_folder) -fdump-ipa-profile

ditto.


And BTW would it make sense to add -gtoggle to stage2 in bootstrap-lto?

diff --git a/config/bootstrap-lto.mk b/config/bootstrap-lto.mk
index 50b86ef1c81..c0cdee69288 100644
--- a/config/bootstrap-lto.mk
+++ b/config/bootstrap-lto.mk
@@ -1,8 +1,8 @@
 # This option enables LTO for stage2 and stage3 in slim mode
 
-STAGE2_CFLAGS += -flto=jobserver -frandom-seed=1
+STAGE2_CFLAGS += -flto=jobserver -frandom-seed=1 -gtoggle
 STAGE3_CFLAGS += -flto=jobserver -frandom-seed=1
-STAGEprofile_CFLAGS += -flto=jobserver -frandom-seed=1
+STAGEprofile_CFLAGS += -flto=jobserver -frandom-seed=1 -gtoggle
 STAGEtrain_CFLAGS += -flto=jobserver -frandom-seed=1
 STAGEfeedback_CFLAGS += -flto=jobserver -frandom-seed=1
Martin Liška Oct. 27, 2017, 1:03 p.m. UTC | #5 
On 10/25/2017 02:19 PM, Markus Trippelsdorf wrote:
> On 2017.08.30 at 11:45 +0200, Martin Liška wrote:
>> diff --git a/Makefile.in b/Makefile.in
>> index 78db0982ba2..16b76906ad0 100644
>> --- a/Makefile.in
>> +++ b/Makefile.in
>> @@ -529,13 +529,14 @@ STAGE1_CONFIGURE_FLAGS = --disable-intermodule $(STAGE1_CHECKING) \
>>   	  --disable-coverage --enable-languages="$(STAGE1_LANGUAGES)" \
>>   	  --disable-build-format-warnings
>>   
>> -STAGEprofile_CFLAGS = $(STAGE2_CFLAGS) -fprofile-generate
>> +profile_folder=`${PWD_COMMAND}`/gcov-profiles/
>> +STAGEprofile_CFLAGS = $(STAGE2_CFLAGS) -fprofile-generate=$(profile_folder)
>>   STAGEprofile_TFLAGS = $(STAGE2_TFLAGS)
>>   
>>   STAGEtrain_CFLAGS = $(STAGE3_CFLAGS)
>>   STAGEtrain_TFLAGS = $(STAGE3_TFLAGS)
>>   
>> -STAGEfeedback_CFLAGS = $(STAGE4_CFLAGS) -fprofile-use
>> +STAGEfeedback_CFLAGS = $(STAGE4_CFLAGS) -fprofile-use=$(profile_folder) -fdump-ipa-profile
> 
> -fdump-ipa-profile looks like a debugging leftover and should be
> dropped.

Sure. Let me prepare a new version of patch.

> 
>>   STAGEfeedback_TFLAGS = $(STAGE4_TFLAGS)
>>   
>>   STAGEautoprofile_CFLAGS = $(STAGE2_CFLAGS) -g
>> diff --git a/Makefile.tpl b/Makefile.tpl
>> index 5fcd7e358d9..129175a579c 100644
>> --- a/Makefile.tpl
>> +++ b/Makefile.tpl
>> @@ -452,13 +452,14 @@ STAGE1_CONFIGURE_FLAGS = --disable-intermodule $(STAGE1_CHECKING) \
>>   	  --disable-coverage --enable-languages="$(STAGE1_LANGUAGES)" \
>>   	  --disable-build-format-warnings
>>   
>> -STAGEprofile_CFLAGS = $(STAGE2_CFLAGS) -fprofile-generate
>> +profile_folder=`${PWD_COMMAND}`/gcov-profiles/
>> +STAGEprofile_CFLAGS = $(STAGE2_CFLAGS) -fprofile-generate=$(profile_folder)
>>   STAGEprofile_TFLAGS = $(STAGE2_TFLAGS)
>>   
>>   STAGEtrain_CFLAGS = $(STAGE3_CFLAGS)
>>   STAGEtrain_TFLAGS = $(STAGE3_TFLAGS)
>>   
>> -STAGEfeedback_CFLAGS = $(STAGE4_CFLAGS) -fprofile-use
>> +STAGEfeedback_CFLAGS = $(STAGE4_CFLAGS) -fprofile-use=$(profile_folder) -fdump-ipa-profile
> 
> ditto.
> 
> 
> And BTW would it make sense to add -gtoggle to stage2 in bootstrap-lto?

Why do you want to have it there? Am I right that we do not do a stage comparison
with LTO bootstrap?

Martin

> 
> diff --git a/config/bootstrap-lto.mk b/config/bootstrap-lto.mk
> index 50b86ef1c81..c0cdee69288 100644
> --- a/config/bootstrap-lto.mk
> +++ b/config/bootstrap-lto.mk
> @@ -1,8 +1,8 @@
>   # This option enables LTO for stage2 and stage3 in slim mode
>   
> -STAGE2_CFLAGS += -flto=jobserver -frandom-seed=1
> +STAGE2_CFLAGS += -flto=jobserver -frandom-seed=1 -gtoggle
>   STAGE3_CFLAGS += -flto=jobserver -frandom-seed=1
> -STAGEprofile_CFLAGS += -flto=jobserver -frandom-seed=1
> +STAGEprofile_CFLAGS += -flto=jobserver -frandom-seed=1 -gtoggle
>   STAGEtrain_CFLAGS += -flto=jobserver -frandom-seed=1
>   STAGEfeedback_CFLAGS += -flto=jobserver -frandom-seed=1
>   
>
Markus Trippelsdorf Oct. 27, 2017, 2:31 p.m. UTC | #6 
On 2017.10.27 at 15:03 +0200, Martin Liška wrote:
> > And BTW would it make sense to add -gtoggle to stage2 in bootstrap-lto?
> 
> Why do you want to have it there? Am I right that we do not do a stage
> comparison with LTO bootstrap?

The idea was to trigger -g -flto at least during one stage, just as a
sanity check. 
Comparison wouldn't make sense, because we would compare LTO object
files.
Richard Biener Oct. 30, 2017, 11:03 a.m. UTC | #7 
On Fri, Oct 27, 2017 at 4:31 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2017.10.27 at 15:03 +0200, Martin Liška wrote:
>> > And BTW would it make sense to add -gtoggle to stage2 in bootstrap-lto?
>>
>> Why do you want to have it there? Am I right that we do not do a stage
>> comparison with LTO bootstrap?
>
> The idea was to trigger -g -flto at least during one stage, just as a
> sanity check.
> Comparison wouldn't make sense, because we would compare LTO object
> files.

I've always wanted to make us compare the actual binaries now that the checksums
do not cause miscompares...  of course linker behavior will then
factor in and I'm
not sure how reliable such compare would be.

And yes, at the moment we _do_ compare LTO bytecode... (which obviously will
fail with -g vs. -g0)

Richard.

> --
> Markus
diff mbox series

Patch

From 654ca05a0e1e0261a4477283ca2dd8678f62f1e7 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 16 Aug 2017 10:22:57 +0200
Subject: [PATCH 1/2] Append PWD to path when using
 -fprofile-generate=/some/path.

---
 gcc/coverage.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/gcc/coverage.c b/gcc/coverage.c
index ed469107e3e..5780e19bbc8 100644
--- a/gcc/coverage.c
+++ b/gcc/coverage.c
@@ -1220,8 +1220,24 @@  coverage_init (const char *filename)
     g->get_passes ()->get_pass_profile ()->static_pass_number;
   g->get_dumps ()->dump_start (profile_pass_num, NULL);
 
-  if (!profile_data_prefix && !IS_ABSOLUTE_PATH (filename))
-    profile_data_prefix = getpwd ();
+  if (!IS_ABSOLUTE_PATH (filename))
+    {
+      if (profile_data_prefix)
+	{
+	  const char *pwd = getpwd ();
+	  unsigned l1 = strlen (profile_data_prefix);
+	  unsigned l2 = strlen (pwd);
+
+	  char *b = XNEWVEC (char, l1 + l2 + 2);
+	  memcpy (b, profile_data_prefix, l1);
+	  b[l1] = '/';
+	  memcpy (b + l1 + 1, pwd, l2);
+	  b[l1 + l2 + 1] = '\0';
+	  profile_data_prefix = b;
+	}
+      else
+	profile_data_prefix = getpwd ();
+    }
 
   if (profile_data_prefix)
     prefix_len = strlen (profile_data_prefix);
-- 
2.14.1