| Message ID | 54B54936.8060600@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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,
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