[patch] update function comments for lto_symtab_encoder_encode_*

[patch] update function comments for lto_symtab_encoder_encode_*

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

Hi Richard.

I'm chasing my tail here looking at an LTO + debug problem, and for the 
life of me I can't figure out how all this partition business affects a 
symbol's `analyzed' bit.  Anyways... the documentation for all these 
functions is wrong.

Can you look at this patch and tell me if it makes sense?  I feel a bit 
uneasy committing under the obvious rule, since I don't entirely 
understand the partitioning thing.

Would anyone mind me fixing this on mainline?  It's just a comment fix.

Also, since you seem to understand all this best, can you suggest some 
better wording for the lto_encoder_entry comments?

/* Entry of LTO symtab encoder.  */
struct lto_encoder_entry
{
   symtab_node *node;
   /* Is the node in this partition (i.e. ltrans of this partition will
      be responsible for outputting it)? */
   unsigned int in_partition:1;
   /* Do we encode body in this partition?  */
   unsigned int body:1;
   /* Do we encode initializer in this partition?
      For example the readonly variable initializers are encoded to aid
      constant folding even if they are not in the partition.  */
   unsigned int initializer:1;
};

Whenever I get to the LTO part of this project, I promise to start 
documenting things better.  This whole thing is a mystery.

Thanks.
Aldy

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2164 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index bddb9a8..68f8042 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2015-01-13  Aldy Hernandez  <aldyh@redhat.com>
+
+	* lto-cgraph: Update function comments for
+	lto_symtab_encoder_encode_*.
+
 2014-12-20  Martin Uecker <uecker@eecs.berkeley.edu>
 
 	* doc/invoke.texi: Document -Wdiscarded-array-qualifiers.
diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index cf92892..0b9d945 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -187,7 +187,7 @@ lto_symtab_encoder_delete_node (lto_symtab_encoder_t encoder,
 }
 
 
-/* Return TRUE if we should encode initializer of NODE (if any).  */
+/* Return TRUE if we should encode the body of NODE (if any).  */
 
 bool
 lto_symtab_encoder_encode_body_p (lto_symtab_encoder_t encoder,
@@ -197,7 +197,7 @@ lto_symtab_encoder_encode_body_p (lto_symtab_encoder_t encoder,
   return encoder->nodes[index].body;
 }
 
-/* Return TRUE if we should encode body of NODE (if any).  */
+/* Specify that we encode the body of NODE in this partition.  */
 
 static void
 lto_set_symtab_encoder_encode_body (lto_symtab_encoder_t encoder,
@@ -220,7 +220,7 @@ lto_symtab_encoder_encode_initializer_p (lto_symtab_encoder_t encoder,
   return encoder->nodes[index].initializer;
 }
 
-/* Return TRUE if we should encode initializer of NODE (if any).  */
+/* Specify that we should encode initializer of NODE (if any).  */
 
 static void
 lto_set_symtab_encoder_encode_initializer (lto_symtab_encoder_t encoder,
@@ -230,7 +230,7 @@ lto_set_symtab_encoder_encode_initializer (lto_symtab_encoder_t encoder,
   encoder->nodes[index].initializer = true;
 }
 
-/* Return TRUE if we should encode initializer of NODE (if any).  */
+/* Return TRUE if NODE is in this partition.  */
 
 bool
 lto_symtab_encoder_in_partition_p (lto_symtab_encoder_t encoder,
@@ -242,7 +242,7 @@ lto_symtab_encoder_in_partition_p (lto_symtab_encoder_t encoder,
   return encoder->nodes[index].in_partition;
 }
 
-/* Return TRUE if we should encode body of NODE (if any).  */
+/* Specify that NODE is in this partition.  */
 
 void
 lto_set_symtab_encoder_in_partition (lto_symtab_encoder_t encoder,

Re: [patch] update function comments for lto_symtab_encoder_encode_*

[Richard Biener]

On Tue, Jan 13, 2015 at 5:35 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> Hi Richard.
>
> I'm chasing my tail here looking at an LTO + debug problem, and for the life
> of me I can't figure out how all this partition business affects a symbol's
> `analyzed' bit.  Anyways... the documentation for all these functions is
> wrong.
>
> Can you look at this patch and tell me if it makes sense?  I feel a bit
> uneasy committing under the obvious rule, since I don't entirely understand
> the partitioning thing.
>
> Would anyone mind me fixing this on mainline?  It's just a comment fix.

Yeah, it's ok for trunk.

> Also, since you seem to understand all this best, can you suggest some
> better wording for the lto_encoder_entry comments?
>
> /* Entry of LTO symtab encoder.  */
> struct lto_encoder_entry
> {
>   symtab_node *node;
>   /* Is the node in this partition (i.e. ltrans of this partition will
>      be responsible for outputting it)? */
>   unsigned int in_partition:1;
>   /* Do we encode body in this partition?  */
>   unsigned int body:1;
>   /* Do we encode initializer in this partition?
>      For example the readonly variable initializers are encoded to aid
>      constant folding even if they are not in the partition.  */
>   unsigned int initializer:1;
> };
>
> Whenever I get to the LTO part of this project, I promise to start
> documenting things better.  This whole thing is a mystery.

Well - mostly to me as well ;)  I'll let Honza answer this...

Thanks,
Richard.

> Thanks.
> Aldy

Re: [patch] update function comments for lto_symtab_encoder_encode_*

[Aldy Hernandez]

On 01/14/2015 01:06 AM, Richard Biener wrote:

>> Whenever I get to the LTO part of this project, I promise to start
>> documenting things better.  This whole thing is a mystery.
>
> Well - mostly to me as well ;)  I'll let Honza answer this...

Ha, you're being too modest!  I get the feeling that no one wants to own 
up to LTO :).

So...

Would anyone mind if I removed all references of "WHOPR" in the 
documentation (doc/lto.texi) and in *most* of the comments in the 
source?  AFAICT, WHOPR has been the default LTO mode since Richard's 
linker plugin patch here:

https://gcc.gnu.org/ml/gcc-patches/2014-03/msg00157.html

 From what I can see, WHOPR is the default unless no partitions were 
found, but otherwise there is no way to disable it.  It's just confusing 
to have this nomenclature that is mostly not applicable.

I obviously wouldn't change actual code, since we're past stage1, but 
comments/documentation are fair game.  Eventually, I'd like to change 
the code to something like "LTO partitioning mode" or something (at the 
next stage1).

Would this be acceptable?

Aldy

Re: [patch] update function comments for lto_symtab_encoder_encode_*

[Richard Biener]

On Wed, Jan 14, 2015 at 5:58 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> On 01/14/2015 01:06 AM, Richard Biener wrote:
>
>>> Whenever I get to the LTO part of this project, I promise to start
>>> documenting things better.  This whole thing is a mystery.
>>
>>
>> Well - mostly to me as well ;)  I'll let Honza answer this...
>
>
> Ha, you're being too modest!  I get the feeling that no one wants to own up
> to LTO :).
>
> So...
>
> Would anyone mind if I removed all references of "WHOPR" in the
> documentation (doc/lto.texi) and in *most* of the comments in the source?
> AFAICT, WHOPR has been the default LTO mode since Richard's linker plugin
> patch here:
>
> https://gcc.gnu.org/ml/gcc-patches/2014-03/msg00157.html
>
> From what I can see, WHOPR is the default unless no partitions were found,
> but otherwise there is no way to disable it.  It's just confusing to have
> this nomenclature that is mostly not applicable.

You can disable WHOPR with -flto-partition=none, otherwise partitions
are always "found" (thus even if we identify only a single partition
we use a separate ltrans process).

> I obviously wouldn't change actual code, since we're past stage1, but
> comments/documentation are fair game.  Eventually, I'd like to change the
> code to something like "LTO partitioning mode" or something (at the next
> stage1).
>
> Would this be acceptable?

I'm not sure what you propose to change?  The references to "WHOPR"
may be historical (refering to the design document), and re-wording
the user-level and internals documentation to make it the default behavior
and maybe cite non-whopr mode as optimization in case of a single
partition is ok.

Note that we still have the issue that we want to exercise both
WHOPR and non-WHOPR in the testsuite but all testcases are so
small that we'd automagically would use non-WHOPR mode (if
such automatism was implemented...).

Richard.

> Aldy

Re: [patch] update function comments for lto_symtab_encoder_encode_*

[Ilya Verbin]

On 15 Jan 09:40, Richard Biener wrote:
> Note that we still have the issue that we want to exercise both
> WHOPR and non-WHOPR in the testsuite but all testcases are so
> small that we'd automagically would use non-WHOPR mode (if
> such automatism was implemented...).

Several tests in libgomp testsuite are quite big to have multiple partitions
(but currently they are tested without -flto), e.g. for-3.c

$ make check-target-libgomp RUNTESTFLAGS="c.exp=for-3.c --target_board=unix/-flto/-save-temps"
$ ls ./x86_64-unknown-linux-gnu/libgomp/testsuite/for-3.exe.ltrans*.o | wc
32

  -- Ilya