| Message ID | 20200101122848.GE10088@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Fix up -Wsuggest-attribute=cold (PR ipa/93087) | expand |
> Hi! > > While for other -Wsuggest-attribute= cases we only warn if the corresponding > attribute is not present on the current_function_decl, enforced in the > callers of warn_function_*, for the cold attribute warn_function_cold is > called in two places in compute_function_frequency, and in the first one > it is only called when the cold attribute is present on the function. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for > trunk? OK. > > The only thing I'm unsure about is whether the other warn_function_cold call > in the same function could be reached if current_function_decl has "cold" > attribute; I haven't managed to construct a testcase where it would, so > perhaps further changes aren't needed, but if that spot could be reachable > even with functions declared with cold attribute, perhaps we'd need > if (lookup_attribute ("cold", DECL_ATTRIBTES (current_function_decl)) == NULL_TREE) > guarding condition for the other warn_function_cold call. I think the code below is also wrong. It first sets frequency to COLD and then updates it to other frequencies. I think warn call needs to be moved after the loop and also predicated by existence of cold attribute. Also the function frequency is computed multiple times, so we need to bookkeep if warning was already emitted. Honza > > 2020-01-01 Jakub Jelinek <jakub@redhat.com> > > PR ipa/93087 > * predict.c (compute_function_frequency): Don't call > warn_function_cold on functions that already have cold attribute. > > * c-c++-common/cold-1.c: New test. > > --- gcc/predict.c.jj 2019-12-10 21:34:48.377594665 +0100 > +++ gcc/predict.c 2019-12-31 17:01:09.807440029 +0100 > @@ -3937,10 +3937,7 @@ compute_function_frequency (void) > int flags = flags_from_decl_or_type (current_function_decl); > if (lookup_attribute ("cold", DECL_ATTRIBUTES (current_function_decl)) > != NULL) > - { > - node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED; > - warn_function_cold (current_function_decl); > - } > + node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED; > else if (lookup_attribute ("hot", DECL_ATTRIBUTES (current_function_decl)) > != NULL) > node->frequency = NODE_FREQUENCY_HOT; > --- gcc/testsuite/c-c++-common/cold-1.c.jj 2019-12-31 17:04:35.154357929 +0100 > +++ gcc/testsuite/c-c++-common/cold-1.c 2019-12-31 17:03:57.992915598 +0100 > @@ -0,0 +1,22 @@ > +/* PR ipa/93087 */ > +/* { dg-do compile { target nonpic } } */ > +/* { dg-options "-O1 -Wsuggest-attribute=cold" } */ > + > +extern void *getdata (void); > +extern int set_error (char const *message) __attribute__((cold)); > + > +__attribute__((cold)) int > +set_nomem (void) /* { dg-bogus "function might be candidate for attribute 'cold'" } */ > +{ > + return set_error ("Allocation failed"); > +} > + > +void * > +getdata_or_set_error (void) > +{ > + void *result; > + result = getdata (); > + if (!result) > + set_nomem (); > + return result; > +} > > Jakub >
--- gcc/predict.c.jj 2019-12-10 21:34:48.377594665 +0100 +++ gcc/predict.c 2019-12-31 17:01:09.807440029 +0100 @@ -3937,10 +3937,7 @@ compute_function_frequency (void) int flags = flags_from_decl_or_type (current_function_decl); if (lookup_attribute ("cold", DECL_ATTRIBUTES (current_function_decl)) != NULL) - { - node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED; - warn_function_cold (current_function_decl); - } + node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED; else if (lookup_attribute ("hot", DECL_ATTRIBUTES (current_function_decl)) != NULL) node->frequency = NODE_FREQUENCY_HOT; --- gcc/testsuite/c-c++-common/cold-1.c.jj 2019-12-31 17:04:35.154357929 +0100 +++ gcc/testsuite/c-c++-common/cold-1.c 2019-12-31 17:03:57.992915598 +0100 @@ -0,0 +1,22 @@ +/* PR ipa/93087 */ +/* { dg-do compile { target nonpic } } */ +/* { dg-options "-O1 -Wsuggest-attribute=cold" } */ + +extern void *getdata (void); +extern int set_error (char const *message) __attribute__((cold)); + +__attribute__((cold)) int +set_nomem (void) /* { dg-bogus "function might be candidate for attribute 'cold'" } */ +{ + return set_error ("Allocation failed"); +} + +void * +getdata_or_set_error (void) +{ + void *result; + result = getdata (); + if (!result) + set_nomem (); + return result; +}
Hi! While for other -Wsuggest-attribute= cases we only warn if the corresponding attribute is not present on the current_function_decl, enforced in the callers of warn_function_*, for the cold attribute warn_function_cold is called in two places in compute_function_frequency, and in the first one it is only called when the cold attribute is present on the function. Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? The only thing I'm unsure about is whether the other warn_function_cold call in the same function could be reached if current_function_decl has "cold" attribute; I haven't managed to construct a testcase where it would, so perhaps further changes aren't needed, but if that spot could be reachable even with functions declared with cold attribute, perhaps we'd need if (lookup_attribute ("cold", DECL_ATTRIBTES (current_function_decl)) == NULL_TREE) guarding condition for the other warn_function_cold call. 2020-01-01 Jakub Jelinek <jakub@redhat.com> PR ipa/93087 * predict.c (compute_function_frequency): Don't call warn_function_cold on functions that already have cold attribute. * c-c++-common/cold-1.c: New test. Jakub