[GOOGLE] Do not partition the cold/hot sections of a function with "section" attribute
This currently puts split sections together again in the specified section and breaks DWARF output. This patch disables the partitioning for such functions. -- 2014-08-06 Yi Yang gcc: * bb-reorder.c (gate_handle_partition_blocks): Add a check for "section" attribute. diff --git gcc/bb-reorder.c gcc/bb-reorder.c index fa6f62f..09449c6 100644 --- gcc/bb-reorder.c +++ gcc/bb-reorder.c @@ -2555,6 +2555,7 @@ gate_handle_partition_blocks (void) we are going to omit the reordering. */ && optimize_function_for_speed_p (cfun) && !DECL_ONE_ONLY (current_function_decl) + && !DECL_SECTION_NAME (current_function_decl) && !user_defined_section_attribute); }
[PATCH] Remove a redundant statement in predict.c
Remove a redundant assignment "*predictor = PRED_BUILTIN_EXPECT;",
since six lines later *predictor is assigned again.
--
2014-08-07 Yi Yang
gcc:
* predict.c (expr_expected_value_1): Remove the redundant assignment.
diff --git gcc/predict.c gcc/predict.c
index 835c618..869fc5d 100644
--- gcc/predict.c
+++ gcc/predict.c
@@ -1858,7 +1858,6 @@ expr_expected_value_1 (tree type, tree op0, enum
tree_code code,
return val;
if (predictor)
{
- *predictor = PRED_BUILTIN_EXPECT;
tree val2 = gimple_call_arg (def, 2);
gcc_assert (TREE_CODE (val2) == INTEGER_CST
&& tree_fits_uhwi_p (val2)
Re: [GOOGLE] Do not partition the cold/hot sections of a function with "section" attribute
Thanks for pointing out this!
It seems to me that this user_defined_section_attribute variable is
useless now and should be removed. It is intended to work in this way:
for each function {
parse into tree (setting user_defined_section_attribute)
do tree passes
do RTL passes (using user_defined_section_attribute)
resetting (user_defined_section_attribute = false)
}
But now GCC works this way:
for each function {
parse into tree (setting user_defined_section_attribute)
}
do IPA passes
for each function {
do tree passes
do RTL passes (using user_defined_section_attribute)
resetting (user_defined_section_attribute = false)
}
Therefore the first function will use the actual
user_defined_section_attribute of the last function, and all other
functions will always see user_defined_section_attribute=0.
I suggest removing user_defined_section_attribute and simply check
!DECL_SECTION_NAME (current_function_decl).
On Mon, Aug 11, 2014 at 8:00 AM, Teresa Johnson wrote:
> On Fri, Aug 8, 2014 at 3:22 PM, Yi Yang wrote:
>> Friendly ping.
>
> Sorry, was OOO.
>
> The solution of preventing splitting for named sections is good - but
> it looks like there is already code that should prevent this. See the
> user_defined_section_attribute check here - why is that not set? Looks
> like it should be set in handle_section_attribute() in
> c-family/c-common.c.
>
> Teresa
>
>>
>>
>> On Wed, Aug 6, 2014 at 5:20 PM, Dehao Chen wrote:
>>>
>>> OK for google-4_8 and google-4_9. David and Teresa may have further
>>> comments.
>>>
>>> Dehao
>>>
>>> On Wed, Aug 6, 2014 at 3:36 PM, Yi Yang wrote:
>>> > This currently puts split sections together again in the specified
>>> > section and breaks DWARF output. This patch disables the partitioning
>>> > for such functions.
>>> >
>>> > --
>>> >
>>> > 2014-08-06 Yi Yang
>>> >
>>> > gcc:
>>> > * bb-reorder.c (gate_handle_partition_blocks): Add a check for
>>> > "section"
>>> > attribute.
>>> >
>>> > diff --git gcc/bb-reorder.c gcc/bb-reorder.c
>>> > index fa6f62f..09449c6 100644
>>> > --- gcc/bb-reorder.c
>>> > +++ gcc/bb-reorder.c
>>> > @@ -2555,6 +2555,7 @@ gate_handle_partition_blocks (void)
>>> > we are going to omit the reordering. */
>>> >&& optimize_function_for_speed_p (cfun)
>>> >&& !DECL_ONE_ONLY (current_function_decl)
>>> > + && !DECL_SECTION_NAME (current_function_decl)
>>> >&& !user_defined_section_attribute);
>>> > }
>>
>>
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [GOOGLE] Do not partition the cold/hot sections of a function with "section" attribute
Patch v2
..
diff --git gcc/bb-reorder.c gcc/bb-reorder.c
index fa6f62f..a1b3e65 100644
--- gcc/bb-reorder.c
+++ gcc/bb-reorder.c
@@ -95,7 +95,6 @@
#include "expr.h"
#include "params.h"
#include "diagnostic-core.h"
-#include "toplev.h" /* user_defined_section_attribute */
#include "tree-pass.h"
#include "df.h"
#include "bb-reorder.h"
@@ -2555,7 +2554,7 @@ gate_handle_partition_blocks (void)
we are going to omit the reordering. */
&& optimize_function_for_speed_p (cfun)
&& !DECL_ONE_ONLY (current_function_decl)
- && !user_defined_section_attribute);
+ && !DECL_SECTION_NAME (current_function_decl));
}
/* This function is the main 'entrance' for the optimization that
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 65c25bf..9923928 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -7378,8 +7378,6 @@ handle_section_attribute (tree *node, tree
ARG_UNUSED (name), tree args,
if (targetm_common.have_named_sections)
{
- user_defined_section_attribute = true;
-
if ((TREE_CODE (decl) == FUNCTION_DECL
|| TREE_CODE (decl) == VAR_DECL)
&& TREE_CODE (TREE_VALUE (args)) == STRING_CST)
diff --git gcc/final.c gcc/final.c
index 9af0b2b..38c90b2 100644
--- gcc/final.c
+++ gcc/final.c
@@ -4501,8 +4501,6 @@ rest_of_handle_final (void)
assemble_end_function (current_function_decl, fnname);
- user_defined_section_attribute = false;
-
/* Free up reg info memory. */
free_reg_info ();
diff --git gcc/toplev.c gcc/toplev.c
index 9b8d313..4c8c965 100644
--- gcc/toplev.c
+++ gcc/toplev.c
@@ -153,11 +153,6 @@ HOST_WIDE_INT random_seed;
the support provided depends on the backend. */
rtx stack_limit_rtx;
-/* True if the user has tagged the function with the 'section'
- attribute. */
-
-bool user_defined_section_attribute = false;
-
struct target_flag_state default_target_flag_state;
#if SWITCHABLE_TARGET
struct target_flag_state *this_target_flag_state = &default_target_flag_state;
diff --git gcc/toplev.h gcc/toplev.h
index 0290be3..65e38e7 100644
--- gcc/toplev.h
+++ gcc/toplev.h
@@ -53,11 +53,6 @@ extern void target_reinit (void);
/* A unique local time stamp, might be zero if none is available. */
extern unsigned local_tick;
-/* True if the user has tagged the function with the 'section'
- attribute. */
-
-extern bool user_defined_section_attribute;
-
/* See toplev.c. */
extern int flag_rerun_cse_after_global_opts;
--
On Mon, Aug 11, 2014 at 12:22 PM, Yi Yang wrote:
> Thanks for pointing out this!
>
> It seems to me that this user_defined_section_attribute variable is
> useless now and should be removed. It is intended to work in this way:
>
> for each function {
>parse into tree (setting user_defined_section_attribute)
>do tree passes
>do RTL passes (using user_defined_section_attribute)
>resetting (user_defined_section_attribute = false)
> }
>
> But now GCC works this way:
>
> for each function {
>parse into tree (setting user_defined_section_attribute)
> }
> do IPA passes
> for each function {
>do tree passes
>do RTL passes (using user_defined_section_attribute)
>resetting (user_defined_section_attribute = false)
> }
>
> Therefore the first function will use the actual
> user_defined_section_attribute of the last function, and all other
> functions will always see user_defined_section_attribute=0.
>
> I suggest removing user_defined_section_attribute and simply check
> !DECL_SECTION_NAME (current_function_decl).
>
> On Mon, Aug 11, 2014 at 8:00 AM, Teresa Johnson wrote:
>> On Fri, Aug 8, 2014 at 3:22 PM, Yi Yang wrote:
>>> Friendly ping.
>>
>> Sorry, was OOO.
>>
>> The solution of preventing splitting for named sections is good - but
>> it looks like there is already code that should prevent this. See the
>> user_defined_section_attribute check here - why is that not set? Looks
>> like it should be set in handle_section_attribute() in
>> c-family/c-common.c.
>>
>> Teresa
>>
>>>
>>>
>>> On Wed, Aug 6, 2014 at 5:20 PM, Dehao Chen wrote:
>>>>
>>>> OK for google-4_8 and google-4_9. David and Teresa may have further
>>>> comments.
>>>>
>>>> Dehao
>>>>
>>>> On Wed, Aug 6, 2014 at 3:36 PM, Yi Yang wrote:
>>>> > This currently puts split sections together again in the specified
>>>> > section and breaks DWARF output. This patch disables the partitioning
>>>> > for such functions.
>>>> >
>>>> > --
>>>> >
>>>> > 2014-08-06 Yi Yang
>>>> &
[PATCH] Drop user_defined_section_attribute, directly check DECL_SECTION_NAME instead
Replace checking user_defined_section_attribute with directly checking
DECL_SECTION_NAME. The former does not work in the presence of IPA.
See also: discussion on the same patch in Google branch:
https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00749.html
--
2014-08-11 Yi Yang
gcc:
* bb-reorder.c (pass_partition_blocks::gate): Replace check.
* c-family/c-common.c (handle_section_attribute): Remove
user_defined_section_attribute
* final.c (rest_of_handle_final): ditto
* toplev.c (user_defined_section_attribute): ditto
* toplev.h (user_defined_section_attribute): ditto
diff --git gcc/bb-reorder.c gcc/bb-reorder.c
index 96547c2..747831c 100644
--- gcc/bb-reorder.c
+++ gcc/bb-reorder.c
@@ -95,7 +95,6 @@
#include "expr.h"
#include "params.h"
#include "diagnostic-core.h"
-#include "toplev.h" /* user_defined_section_attribute */
#include "tree-pass.h"
#include "df.h"
#include "bb-reorder.h"
@@ -2671,11 +2670,9 @@ pass_partition_blocks::gate (function *fun)
arises. */
return (flag_reorder_blocks_and_partition
&& optimize
- /* See gate_handle_reorder_blocks. We should not partition if
- we are going to omit the reordering. */
&& optimize_function_for_speed_p (fun)
- && !DECL_COMDAT_GROUP (current_function_decl)
- && !user_defined_section_attribute);
+ && !DECL_COMDAT_GROUP (current_function_decl);
+ && !DECL_SECTION_NAME (current_function_decl));
}
unsigned
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index acc9a20..967ae2b 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -7395,8 +7395,6 @@ handle_section_attribute (tree *node, tree
ARG_UNUSED (name), tree args,
if (targetm_common.have_named_sections)
{
- user_defined_section_attribute = true;
-
if ((TREE_CODE (decl) == FUNCTION_DECL
|| TREE_CODE (decl) == VAR_DECL)
&& TREE_CODE (TREE_VALUE (args)) == STRING_CST)
diff --git gcc/final.c gcc/final.c
index 304ae2a..3fee226 100644
--- gcc/final.c
+++ gcc/final.c
@@ -4460,8 +4460,6 @@ rest_of_handle_final (void)
assemble_end_function (current_function_decl, fnname);
- user_defined_section_attribute = false;
-
/* Free up reg info memory. */
free_reg_info ();
diff --git gcc/toplev.c gcc/toplev.c
index 88d48c2..07d5e05 100644
--- gcc/toplev.c
+++ gcc/toplev.c
@@ -152,11 +152,6 @@ HOST_WIDE_INT random_seed;
the support provided depends on the backend. */
rtx stack_limit_rtx;
-/* True if the user has tagged the function with the 'section'
- attribute. */
-
-bool user_defined_section_attribute = false;
-
struct target_flag_state default_target_flag_state;
#if SWITCHABLE_TARGET
struct target_flag_state *this_target_flag_state = &default_target_flag_state;
diff --git gcc/toplev.h gcc/toplev.h
index 1b54578..b0d0ca4 100644
--- gcc/toplev.h
+++ gcc/toplev.h
@@ -53,11 +53,6 @@ extern void target_reinit (void);
/* A unique local time stamp, might be zero if none is available. */
extern unsigned local_tick;
-/* True if the user has tagged the function with the 'section'
- attribute. */
-
-extern bool user_defined_section_attribute;
-
/* See toplev.c. */
extern int flag_rerun_cse_after_global_opts;
--
Re: [GOOGLE] Do not partition the cold/hot sections of a function with "section" attribute
I ported this to trunk.
Shall I commit this patch to the Google 4_8/4_9 branches first?
On Mon, Aug 11, 2014 at 12:46 PM, Teresa Johnson wrote:
> Ok, thanks. This seems reasonable. Can you send the patch to trunk as well?
> Teresa
>
> On Mon, Aug 11, 2014 at 12:35 PM, Yi Yang wrote:
>> Patch v2
>>
>> ..
>>
>> diff --git gcc/bb-reorder.c gcc/bb-reorder.c
>> index fa6f62f..a1b3e65 100644
>> --- gcc/bb-reorder.c
>> +++ gcc/bb-reorder.c
>> @@ -95,7 +95,6 @@
>> #include "expr.h"
>> #include "params.h"
>> #include "diagnostic-core.h"
>> -#include "toplev.h" /* user_defined_section_attribute */
>> #include "tree-pass.h"
>> #include "df.h"
>> #include "bb-reorder.h"
>> @@ -2555,7 +2554,7 @@ gate_handle_partition_blocks (void)
>> we are going to omit the reordering. */
>>&& optimize_function_for_speed_p (cfun)
>>&& !DECL_ONE_ONLY (current_function_decl)
>> - && !user_defined_section_attribute);
>> + && !DECL_SECTION_NAME (current_function_decl));
>> }
>>
>> /* This function is the main 'entrance' for the optimization that
>> diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
>> index 65c25bf..9923928 100644
>> --- gcc/c-family/c-common.c
>> +++ gcc/c-family/c-common.c
>> @@ -7378,8 +7378,6 @@ handle_section_attribute (tree *node, tree
>> ARG_UNUSED (name), tree args,
>>
>>if (targetm_common.have_named_sections)
>> {
>> - user_defined_section_attribute = true;
>> -
>>if ((TREE_CODE (decl) == FUNCTION_DECL
>> || TREE_CODE (decl) == VAR_DECL)
>>&& TREE_CODE (TREE_VALUE (args)) == STRING_CST)
>> diff --git gcc/final.c gcc/final.c
>> index 9af0b2b..38c90b2 100644
>> --- gcc/final.c
>> +++ gcc/final.c
>> @@ -4501,8 +4501,6 @@ rest_of_handle_final (void)
>>
>>assemble_end_function (current_function_decl, fnname);
>>
>> - user_defined_section_attribute = false;
>> -
>>/* Free up reg info memory. */
>>free_reg_info ();
>>
>> diff --git gcc/toplev.c gcc/toplev.c
>> index 9b8d313..4c8c965 100644
>> --- gcc/toplev.c
>> +++ gcc/toplev.c
>> @@ -153,11 +153,6 @@ HOST_WIDE_INT random_seed;
>> the support provided depends on the backend. */
>> rtx stack_limit_rtx;
>>
>> -/* True if the user has tagged the function with the 'section'
>> - attribute. */
>> -
>> -bool user_defined_section_attribute = false;
>> -
>> struct target_flag_state default_target_flag_state;
>> #if SWITCHABLE_TARGET
>> struct target_flag_state *this_target_flag_state =
>> &default_target_flag_state;
>> diff --git gcc/toplev.h gcc/toplev.h
>> index 0290be3..65e38e7 100644
>> --- gcc/toplev.h
>> +++ gcc/toplev.h
>> @@ -53,11 +53,6 @@ extern void target_reinit (void);
>> /* A unique local time stamp, might be zero if none is available. */
>> extern unsigned local_tick;
>>
>> -/* True if the user has tagged the function with the 'section'
>> - attribute. */
>> -
>> -extern bool user_defined_section_attribute;
>> -
>> /* See toplev.c. */
>> extern int flag_rerun_cse_after_global_opts;
>>
>> --
>>
>> On Mon, Aug 11, 2014 at 12:22 PM, Yi Yang wrote:
>>> Thanks for pointing out this!
>>>
>>> It seems to me that this user_defined_section_attribute variable is
>>> useless now and should be removed. It is intended to work in this way:
>>>
>>> for each function {
>>>parse into tree (setting user_defined_section_attribute)
>>>do tree passes
>>>do RTL passes (using user_defined_section_attribute)
>>>resetting (user_defined_section_attribute = false)
>>> }
>>>
>>> But now GCC works this way:
>>>
>>> for each function {
>>>parse into tree (setting user_defined_section_attribute)
>>> }
>>> do IPA passes
>>> for each function {
>>>do tree passes
>>>do RTL passes (using user_defined_section_attribute)
>>>resetting (user_defined_section_attribute = false)
>>> }
>>>
>>> Therefore the first function will use the actual
>>> user_defined_section_attribute of the last function, and all other
>>> functions will always see user_defined_section_attribute=0.
>>>
>>> I suggest removing user_defined_section_attribute and si
Re: [PATCH] Drop user_defined_section_attribute, directly check DECL_SECTION_NAME instead
Sorry, it is a typo :(
Patch v2:
--
2014-08-11 Yi Yang
gcc:
* bb-reorder.c (pass_partition_blocks::gate): Replace check.
* c-family/c-common.c (handle_section_attribute): Remove
user_defined_section_attribute
* final.c (rest_of_handle_final): ditto
* toplev.c (user_defined_section_attribute): ditto
* toplev.h (user_defined_section_attribute): ditto
diff --git gcc/bb-reorder.c gcc/bb-reorder.c
index 96547c2..7b74887 100644
--- gcc/bb-reorder.c
+++ gcc/bb-reorder.c
@@ -95,7 +95,6 @@
#include "expr.h"
#include "params.h"
#include "diagnostic-core.h"
-#include "toplev.h" /* user_defined_section_attribute */
#include "tree-pass.h"
#include "df.h"
#include "bb-reorder.h"
@@ -2671,11 +2670,9 @@ pass_partition_blocks::gate (function *fun)
arises. */
return (flag_reorder_blocks_and_partition
&& optimize
- /* See gate_handle_reorder_blocks. We should not partition if
- we are going to omit the reordering. */
&& optimize_function_for_speed_p (fun)
&& !DECL_COMDAT_GROUP (current_function_decl)
- && !user_defined_section_attribute);
+ && !DECL_SECTION_NAME (current_function_decl));
}
unsigned
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index acc9a20..967ae2b 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -7395,8 +7395,6 @@ handle_section_attribute (tree *node, tree
ARG_UNUSED (name), tree args,
if (targetm_common.have_named_sections)
{
- user_defined_section_attribute = true;
-
if ((TREE_CODE (decl) == FUNCTION_DECL
|| TREE_CODE (decl) == VAR_DECL)
&& TREE_CODE (TREE_VALUE (args)) == STRING_CST)
diff --git gcc/final.c gcc/final.c
index 304ae2a..3fee226 100644
--- gcc/final.c
+++ gcc/final.c
@@ -4460,8 +4460,6 @@ rest_of_handle_final (void)
assemble_end_function (current_function_decl, fnname);
- user_defined_section_attribute = false;
-
/* Free up reg info memory. */
free_reg_info ();
diff --git gcc/toplev.c gcc/toplev.c
index 88d48c2..07d5e05 100644
--- gcc/toplev.c
+++ gcc/toplev.c
@@ -152,11 +152,6 @@ HOST_WIDE_INT random_seed;
the support provided depends on the backend. */
rtx stack_limit_rtx;
-/* True if the user has tagged the function with the 'section'
- attribute. */
-
-bool user_defined_section_attribute = false;
-
struct target_flag_state default_target_flag_state;
#if SWITCHABLE_TARGET
struct target_flag_state *this_target_flag_state = &default_target_flag_state;
diff --git gcc/toplev.h gcc/toplev.h
index 1b54578..b0d0ca4 100644
--- gcc/toplev.h
+++ gcc/toplev.h
@@ -53,11 +53,6 @@ extern void target_reinit (void);
/* A unique local time stamp, might be zero if none is available. */
extern unsigned local_tick;
-/* True if the user has tagged the function with the 'section'
- attribute. */
-
-extern bool user_defined_section_attribute;
-
/* See toplev.c. */
extern int flag_rerun_cse_after_global_opts;
--
On Mon, Aug 11, 2014 at 1:46 PM, H.J. Lu wrote:
> On Mon, Aug 11, 2014 at 1:41 PM, Yi Yang wrote:
>> Replace checking user_defined_section_attribute with directly checking
>> DECL_SECTION_NAME. The former does not work in the presence of IPA.
>>
>> See also: discussion on the same patch in Google branch:
>> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00749.html
>>
>> --
>>
>> 2014-08-11 Yi Yang
>>
>> gcc:
>> * bb-reorder.c (pass_partition_blocks::gate): Replace check.
>> * c-family/c-common.c (handle_section_attribute): Remove
>> user_defined_section_attribute
>> * final.c (rest_of_handle_final): ditto
>> * toplev.c (user_defined_section_attribute): ditto
>> * toplev.h (user_defined_section_attribute): ditto
>>
>> diff --git gcc/bb-reorder.c gcc/bb-reorder.c
>> index 96547c2..747831c 100644
>> --- gcc/bb-reorder.c
>> +++ gcc/bb-reorder.c
>> @@ -95,7 +95,6 @@
>> #include "expr.h"
>> #include "params.h"
>> #include "diagnostic-core.h"
>> -#include "toplev.h" /* user_defined_section_attribute */
>> #include "tree-pass.h"
>> #include "df.h"
>> #include "bb-reorder.h"
>> @@ -2671,11 +2670,9 @@ pass_partition_blocks::gate (function *fun)
>> arises. */
>>return (flag_reorder_blocks_and_partition
>>&& optimize
>> - /* See gate_handle_reorder_blocks. We should not partition if
>> - we are going to omit the reordering. */
>>&& optimize_function_for_speed_p (fun)
>> - && !DECL_COMDAT_GROUP (current_function_decl)
>> - && !user_defined_section_attribute);
>> + && !DECL_COMDAT_GROUP (current_function_decl);
>
> ^^^ Is this extra ';' a typo?
>
>> + && !DECL_SECTION_NAME (current_function_decl));
>> }
>>
>
>
>
> --
> H.J.
[GOOGLE] Fix the bug where implicit section names prevents function splitting
This bug is caused by my last patch, which did not differentiate
between explicit section names (via attributes) and implicit section
names (via -ffunction-section).
This patch fixes that.
--
diff --git gcc/bb-reorder.c gcc/bb-reorder.c
index 8f8c420..2115b01 100644
--- gcc/bb-reorder.c
+++ gcc/bb-reorder.c
@@ -2505,7 +2505,7 @@ gate_handle_partition_blocks (void)
we are going to omit the reordering. */
&& optimize_function_for_speed_p (cfun)
&& !DECL_ONE_ONLY (current_function_decl)
- && !DECL_SECTION_NAME (current_function_decl));
+ && !DECL_HAS_EXPLICIT_SECTION_NAME_P(current_function_decl));
}
/* This function is the main 'entrance' for the optimization that
diff --git gcc/tree.h gcc/tree.h
index 817507f..738675a 100644
--- gcc/tree.h
+++ gcc/tree.h
@@ -3201,6 +3201,11 @@ struct GTY(()) tree_parm_decl {
#define DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE) \
(DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.implicit_section_name_p)
+/* Speficy whether the section name was explicitly set with decl_attributes. */
+#define DECL_HAS_EXPLICIT_SECTION_NAME_P(NODE) \
+ (DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE)? false: \
+ !!DECL_SECTION_NAME(NODE))
+
struct GTY(()) tree_decl_with_vis {
struct tree_decl_with_rtl common;
tree assembler_name;
--
Re: [GOOGLE] Fix the bug where implicit section names prevents function splitting
Patch v2.
Trunk no longer set SECTION_NAME for implicit section names, so this
probably does not apply to trunk. It's probably not necessary for
trunk either.
Tested for Google 4.8(albeit unnecessary) and 4.9 branch.
diff --git gcc/bb-reorder.c gcc/bb-reorder.c
index a1b3e65..b9a829e 100644
--- gcc/bb-reorder.c
+++ gcc/bb-reorder.c
@@ -2554,7 +2554,7 @@ gate_handle_partition_blocks (void)
we are going to omit the reordering. */
&& optimize_function_for_speed_p (cfun)
&& !DECL_ONE_ONLY (current_function_decl)
- && !DECL_SECTION_NAME (current_function_decl));
+ && !DECL_HAS_EXPLICIT_SECTION_NAME_P(current_function_decl));
}
/* This function is the main 'entrance' for the optimization that
diff --git gcc/tree.h gcc/tree.h
index b656b7b..308eef8 100644
--- gcc/tree.h
+++ gcc/tree.h
@@ -2405,6 +2405,11 @@ extern void decl_value_expr_insert (tree, tree);
#define DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE) \
(DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.implicit_section_name_p)
+/* Speficy whether the section name was explicitly set with decl_attributes. */
+#define DECL_HAS_EXPLICIT_SECTION_NAME_P(NODE) \
+ (DECL_SECTION_NAME(NODE) != NULL_TREE \
+ && !DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE))
+
extern tree decl_debug_expr_lookup (tree);
extern void decl_debug_expr_insert (tree, tree);
--
On Thu, Aug 14, 2014 at 11:25 AM, Teresa Johnson wrote:
> On Wed, Aug 13, 2014 at 9:03 PM, Yi Yang wrote:
>> This bug is caused by my last patch, which did not differentiate
>> between explicit section names (via attributes) and implicit section
>> names (via -ffunction-section).
>>
>> This patch fixes that.
>>
>> --
>>
>> diff --git gcc/bb-reorder.c gcc/bb-reorder.c
>> index 8f8c420..2115b01 100644
>> --- gcc/bb-reorder.c
>> +++ gcc/bb-reorder.c
>> @@ -2505,7 +2505,7 @@ gate_handle_partition_blocks (void)
>> we are going to omit the reordering. */
>> && optimize_function_for_speed_p (cfun)
>> && !DECL_ONE_ONLY (current_function_decl)
>> - && !DECL_SECTION_NAME (current_function_decl));
>> + && !DECL_HAS_EXPLICIT_SECTION_NAME_P(current_function_decl));
>> }
>>
>> /* This function is the main 'entrance' for the optimization that
>> diff --git gcc/tree.h gcc/tree.h
>> index 817507f..738675a 100644
>> --- gcc/tree.h
>> +++ gcc/tree.h
>> @@ -3201,6 +3201,11 @@ struct GTY(()) tree_parm_decl {
>> #define DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE) \
>>(DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.implicit_section_name_p)
>>
>> +/* Speficy whether the section name was explicitly set with
>> decl_attributes. */
>> +#define DECL_HAS_EXPLICIT_SECTION_NAME_P(NODE) \
>> + (DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE)? false: \
>> + !!DECL_SECTION_NAME(NODE))
>
> Personally, I think it is clearer to simply write this as:
>
> #define DECL_HAS_EXPLICIT_SECTION_NAME_P(NODE) \
> (DECL_SECTION_NAME(NODE) != NULL_TREE \
> && !DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE))
>
> Teresa
>
>> +
>> struct GTY(()) tree_decl_with_vis {
>> struct tree_decl_with_rtl common;
>> tree assembler_name;
>> --
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [GOOGLE] Fix the bug where implicit section names prevents function splitting
Thank you. I fixed the typo and committed.
On Thu, Aug 14, 2014 at 1:49 PM, Teresa Johnson wrote:
> On Thu, Aug 14, 2014 at 1:46 PM, Yi Yang wrote:
>> Patch v2.
>>
>> Trunk no longer set SECTION_NAME for implicit section names, so this
>> probably does not apply to trunk. It's probably not necessary for
>> trunk either.
>>
>> Tested for Google 4.8(albeit unnecessary) and 4.9 branch.
>>
>> diff --git gcc/bb-reorder.c gcc/bb-reorder.c
>> index a1b3e65..b9a829e 100644
>> --- gcc/bb-reorder.c
>> +++ gcc/bb-reorder.c
>> @@ -2554,7 +2554,7 @@ gate_handle_partition_blocks (void)
>> we are going to omit the reordering. */
>>&& optimize_function_for_speed_p (cfun)
>>&& !DECL_ONE_ONLY (current_function_decl)
>> - && !DECL_SECTION_NAME (current_function_decl));
>> + && !DECL_HAS_EXPLICIT_SECTION_NAME_P(current_function_decl));
>> }
>>
>> /* This function is the main 'entrance' for the optimization that
>> diff --git gcc/tree.h gcc/tree.h
>> index b656b7b..308eef8 100644
>> --- gcc/tree.h
>> +++ gcc/tree.h
>> @@ -2405,6 +2405,11 @@ extern void decl_value_expr_insert (tree, tree);
>> #define DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE) \
>>(DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.implicit_section_name_p)
>>
>> +/* Speficy whether the section name was explicitly set with
>> decl_attributes. */
>
> Typo "Specify".
>
> Otherwise looks ok to me.
>
> Thanks,
> Teresa
>
>> +#define DECL_HAS_EXPLICIT_SECTION_NAME_P(NODE) \
>> + (DECL_SECTION_NAME(NODE) != NULL_TREE \
>> + && !DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE))
>> +
>> extern tree decl_debug_expr_lookup (tree);
>> extern void decl_debug_expr_insert (tree, tree);
>>
>> --
>>
>> On Thu, Aug 14, 2014 at 11:25 AM, Teresa Johnson
>> wrote:
>>> On Wed, Aug 13, 2014 at 9:03 PM, Yi Yang wrote:
>>>> This bug is caused by my last patch, which did not differentiate
>>>> between explicit section names (via attributes) and implicit section
>>>> names (via -ffunction-section).
>>>>
>>>> This patch fixes that.
>>>>
>>>> --
>>>>
>>>> diff --git gcc/bb-reorder.c gcc/bb-reorder.c
>>>> index 8f8c420..2115b01 100644
>>>> --- gcc/bb-reorder.c
>>>> +++ gcc/bb-reorder.c
>>>> @@ -2505,7 +2505,7 @@ gate_handle_partition_blocks (void)
>>>> we are going to omit the reordering. */
>>>> && optimize_function_for_speed_p (cfun)
>>>> && !DECL_ONE_ONLY (current_function_decl)
>>>> - && !DECL_SECTION_NAME (current_function_decl));
>>>> + && !DECL_HAS_EXPLICIT_SECTION_NAME_P(current_function_decl));
>>>> }
>>>>
>>>> /* This function is the main 'entrance' for the optimization that
>>>> diff --git gcc/tree.h gcc/tree.h
>>>> index 817507f..738675a 100644
>>>> --- gcc/tree.h
>>>> +++ gcc/tree.h
>>>> @@ -3201,6 +3201,11 @@ struct GTY(()) tree_parm_decl {
>>>> #define DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE) \
>>>>(DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.implicit_section_name_p)
>>>>
>>>> +/* Speficy whether the section name was explicitly set with
>>>> decl_attributes. */
>>>> +#define DECL_HAS_EXPLICIT_SECTION_NAME_P(NODE) \
>>>> + (DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE)? false: \
>>>> + !!DECL_SECTION_NAME(NODE))
>>>
>>> Personally, I think it is clearer to simply write this as:
>>>
>>> #define DECL_HAS_EXPLICIT_SECTION_NAME_P(NODE) \
>>> (DECL_SECTION_NAME(NODE) != NULL_TREE \
>>> && !DECL_HAS_IMPLICIT_SECTION_NAME_P(NODE))
>>>
>>> Teresa
>>>
>>>> +
>>>> struct GTY(()) tree_decl_with_vis {
>>>> struct tree_decl_with_rtl common;
>>>> tree assembler_name;
>>>> --
>>>
>>>
>>>
>>> --
>>> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH] Drop user_defined_section_attribute, directly check DECL_SECTION_NAME instead
Ping
On Mon, Aug 11, 2014 at 3:10 PM, Yi Yang wrote:
> Sorry, it is a typo :(
>
> Patch v2:
>
> --
>
> 2014-08-11 Yi Yang
>
> gcc:
> * bb-reorder.c (pass_partition_blocks::gate): Replace check.
> * c-family/c-common.c (handle_section_attribute): Remove
> user_defined_section_attribute
> * final.c (rest_of_handle_final): ditto
> * toplev.c (user_defined_section_attribute): ditto
> * toplev.h (user_defined_section_attribute): ditto
>
> diff --git gcc/bb-reorder.c gcc/bb-reorder.c
> index 96547c2..7b74887 100644
> --- gcc/bb-reorder.c
> +++ gcc/bb-reorder.c
> @@ -95,7 +95,6 @@
> #include "expr.h"
> #include "params.h"
> #include "diagnostic-core.h"
> -#include "toplev.h" /* user_defined_section_attribute */
> #include "tree-pass.h"
> #include "df.h"
> #include "bb-reorder.h"
> @@ -2671,11 +2670,9 @@ pass_partition_blocks::gate (function *fun)
> arises. */
>return (flag_reorder_blocks_and_partition
>&& optimize
> - /* See gate_handle_reorder_blocks. We should not partition if
> - we are going to omit the reordering. */
>&& optimize_function_for_speed_p (fun)
>&& !DECL_COMDAT_GROUP (current_function_decl)
> - && !user_defined_section_attribute);
> + && !DECL_SECTION_NAME (current_function_decl));
> }
>
> unsigned
> diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
> index acc9a20..967ae2b 100644
> --- gcc/c-family/c-common.c
> +++ gcc/c-family/c-common.c
> @@ -7395,8 +7395,6 @@ handle_section_attribute (tree *node, tree
> ARG_UNUSED (name), tree args,
>
>if (targetm_common.have_named_sections)
> {
> - user_defined_section_attribute = true;
> -
>if ((TREE_CODE (decl) == FUNCTION_DECL
> || TREE_CODE (decl) == VAR_DECL)
>&& TREE_CODE (TREE_VALUE (args)) == STRING_CST)
> diff --git gcc/final.c gcc/final.c
> index 304ae2a..3fee226 100644
> --- gcc/final.c
> +++ gcc/final.c
> @@ -4460,8 +4460,6 @@ rest_of_handle_final (void)
>
>assemble_end_function (current_function_decl, fnname);
>
> - user_defined_section_attribute = false;
> -
>/* Free up reg info memory. */
>free_reg_info ();
>
> diff --git gcc/toplev.c gcc/toplev.c
> index 88d48c2..07d5e05 100644
> --- gcc/toplev.c
> +++ gcc/toplev.c
> @@ -152,11 +152,6 @@ HOST_WIDE_INT random_seed;
> the support provided depends on the backend. */
> rtx stack_limit_rtx;
>
> -/* True if the user has tagged the function with the 'section'
> - attribute. */
> -
> -bool user_defined_section_attribute = false;
> -
> struct target_flag_state default_target_flag_state;
> #if SWITCHABLE_TARGET
> struct target_flag_state *this_target_flag_state =
> &default_target_flag_state;
> diff --git gcc/toplev.h gcc/toplev.h
> index 1b54578..b0d0ca4 100644
> --- gcc/toplev.h
> +++ gcc/toplev.h
> @@ -53,11 +53,6 @@ extern void target_reinit (void);
> /* A unique local time stamp, might be zero if none is available. */
> extern unsigned local_tick;
>
> -/* True if the user has tagged the function with the 'section'
> - attribute. */
> -
> -extern bool user_defined_section_attribute;
> -
> /* See toplev.c. */
> extern int flag_rerun_cse_after_global_opts;
>
> --
>
> On Mon, Aug 11, 2014 at 1:46 PM, H.J. Lu wrote:
>> On Mon, Aug 11, 2014 at 1:41 PM, Yi Yang wrote:
>>> Replace checking user_defined_section_attribute with directly checking
>>> DECL_SECTION_NAME. The former does not work in the presence of IPA.
>>>
>>> See also: discussion on the same patch in Google branch:
>>> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00749.html
>>>
>>> --
>>>
>>> 2014-08-11 Yi Yang
>>>
>>> gcc:
>>> * bb-reorder.c (pass_partition_blocks::gate): Replace check.
>>> * c-family/c-common.c (handle_section_attribute): Remove
>>> user_defined_section_attribute
>>> * final.c (rest_of_handle_final): ditto
>>> * toplev.c (user_defined_section_attribute): ditto
>>> * toplev.h (user_defined_section_attribute): ditto
>>>
>>> diff --git gcc/bb-reorder.c gcc/bb-reorder.c
>>> index 96547c2..747831c 100644
>>> --- gcc/bb-reorder.c
>>> +++ gcc/bb-reorder.c
>>> @@ -95,7 +95,6 @@
>>> #include "expr.h"
>>> #include "params.h"
>>> #include "diagnostic-core.h"
>>> -#include "toplev.h" /* user_defined_section_attribute */
>>> #include "tree-pass.h"
>>> #include "df.h"
>>> #include "bb-reorder.h"
>>> @@ -2671,11 +2670,9 @@ pass_partition_blocks::gate (function *fun)
>>> arises. */
>>>return (flag_reorder_blocks_and_partition
>>>&& optimize
>>> - /* See gate_handle_reorder_blocks. We should not partition if
>>> - we are going to omit the reordering. */
>>>&& optimize_function_for_speed_p (fun)
>>> - && !DECL_COMDAT_GROUP (current_function_decl)
>>> - && !user_defined_section_attribute);
>>> + && !DECL_COMDAT_GROUP (current_function_decl);
>>
>> ^^^ Is this extra ';' a typo?
>>
>>> + && !DECL_SECTION_NAME (current_function_decl));
>>> }
>>>
>>
>>
>>
>> --
>> H.J.
[GOOGLE] Do not change edge probabilities when propagating edge counts
Hi,
This patch removes unnecessary edge probability calculations in
afdo_propagate_circuit() that would eventually be overridden by
afdo_calculate_branch_prob().
This would pave the way for my next patch, which compares the
estimated branch probabilities or the branch annotations against the
real profile data.
Thanks,
Yi
gcc/
2014-06-24 Yi Yang
* auto-profile.c (afdo_propagate_circuit): Do not change edge
probabilities when propagating edge counts
diff --git gcc/auto-profile.c gcc/auto-profile.c
index 51e318d..74d3d1d 100644
--- gcc/auto-profile.c
+++ gcc/auto-profile.c
@@ -1328,16 +1328,9 @@ afdo_propagate_circuit (void)
continue;
total++;
only_one = ep;
- if (e->probability == 0 && (e->flags & EDGE_ANNOTATED) == 0)
- {
- ep->probability = 0;
- ep->count = 0;
- ep->flags |= EDGE_ANNOTATED;
- }
}
if (total == 1 && (only_one->flags & EDGE_ANNOTATED) == 0)
{
- only_one->probability = e->probability;
only_one->count = e->count;
only_one->flags |= EDGE_ANNOTATED;
}
--
[GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
Hi,
This patch adds an option. When the option is enabled, GCC will add a
record about it in an elf section called
".gnu.switches.text.branch.annotation" for every branch.
gcc/
2014-06-27 Yi Yang
* auto-profile.c: Main comparison and reporting logic.
* cfg-flags.def: Add an extra flag representing an edge's
probability is predicted by annotations.
* predict.c: Set up the extra flag on an edge when appropriate.
* common.opt: Add an extra GCC option to turn on this report mechanism
diff --git a/gcc/auto-profile.c b/gcc/auto-profile.c
index 74d3d1d..f7698cd 100644
--- a/gcc/auto-profile.c
+++ b/gcc/auto-profile.c
@@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see
#include "opts.h" /* for in_fnames. */
#include "tree-pass.h" /* for ipa pass. */
#include "cfgloop.h" /* for loop_optimizer_init. */
+#include "md5.h" /* for hashing function names. */
#include "gimple.h"
#include "cgraph.h"
#include "tree-flow.h"
@@ -1367,6 +1368,80 @@ afdo_propagate (void)
}
}
+struct locus_information_t {
+ char filename[1024];
+ unsigned lineno;
+ unsigned function_lineno;
+ char function_hash[33];
+};
+
+static bool
+get_locus_information (location_t locus, locus_information_t* li) {
+ if (locus == UNKNOWN_LOCATION || !LOCATION_FILE(locus))
+return false;
+ snprintf(li->filename, 1024, "%s", LOCATION_FILE(locus));
+ li->lineno = LOCATION_LINE(locus);
+
+ tree block = LOCATION_BLOCK (locus);
+ tree function_decl = current_function_decl;
+
+ if (block && TREE_CODE (block) == BLOCK)
+{
+ for (block = BLOCK_SUPERCONTEXT (block);
+ block && (TREE_CODE (block) == BLOCK);
+ block = BLOCK_SUPERCONTEXT (block))
+ {
+ location_t tmp_locus = BLOCK_SOURCE_LOCATION (block);
+ if (LOCATION_LOCUS (tmp_locus) == UNKNOWN_LOCATION)
+ continue;
+
+ tree decl = get_function_decl_from_block (block);
+ function_decl = decl;
+ break;
+ }
+}
+
+ if (!(function_decl && TREE_CODE (function_decl) == FUNCTION_DECL))
+return false;
+ unsigned function_length = 0;
+ function *f = DECL_STRUCT_FUNCTION(function_decl);
+
+ li->function_lineno = LOCATION_LINE(DECL_SOURCE_LOCATION(function_decl));
+
+ if (f)
+{
+ function_length = LOCATION_LINE(f->function_end_locus) -
+ li->function_lineno;
+}
+
+ const char *fn_name = fndecl_name(function_decl);
+ unsigned char md5_result[16];
+
+ md5_ctx ctx;
+
+ md5_init_ctx(&ctx);
+ md5_process_bytes(fn_name, strlen(fn_name), &ctx);
+ md5_process_bytes(&function_length, sizeof(function_length), &ctx);
+ md5_finish_ctx(&ctx, md5_result);
+
+ for (int i = 0; i < 16; ++i)
+{
+ sprintf(li->function_hash + i*2, "%02x", md5_result[i]);
+}
+
+ return true;
+}
+
+void
+fill_invalid_locus_information(locus_information_t* li) {
+ snprintf(li->filename, 1024, "");
+ li->lineno = 0;
+ li->function_lineno = 0;
+ for (int i = 0; i < 32; ++i)
+li->function_hash[i] = '0';
+ li->function_hash[32] = '\0';
+}
+
/* Propagate counts on control flow graph and calculate branch
probabilities. */
@@ -1407,8 +1482,62 @@ afdo_calculate_branch_prob (void)
if (num_unknown_succ == 0 && total_count > 0)
{
FOR_EACH_EDGE (e, ei, bb->succs)
- e->probability =
- (double) e->count * REG_BR_PROB_BASE / total_count;
+ {
+ double probability =
+ (double) e->count * REG_BR_PROB_BASE / total_count;
+
+ if (flag_check_branch_annotation &&
+ bb->succs->length() == 2 &&
+ maybe_hot_count_p (cfun, bb->count) &&
+ bb->count >= 100)
+ {
+ gimple_stmt_iterator gsi;
+ gimple last = NULL;
+
+ for (gsi = gsi_last_nondebug_bb (bb);
+ !gsi_end_p (gsi);
+ gsi_prev_nondebug (&gsi))
+ {
+ last = gsi_stmt (gsi);
+
+ if (gimple_has_location (last))
+ break;
+ }
+
+ struct locus_information_t li;
+ bool annotated;
+
+ if (e->flags & EDGE_PREDICTED_BY_EXPECT)
+ annotated = true;
+ else
+ annotated = false;
+
+ if (get_locus_information(e->goto_locus, &li))
+ ;
+ else if (get_locus_information(gimple_location(last), &li))
+ ;
+ else
+ fill_invalid_locus_information(&li);
+
+ switch_to_section (get_section (
+ ".gnu.switches.text.branch.annotation",
+ SECTION_DEBUG | SECTION_MERGE |
+ SECTION_STRINGS | (SECTION_ENTSIZE & 1),
+ NULL));
+ char buf[1024];
+ snprintf (buf, 1024, "%s;%u;%d;"
+ HOST_WIDEST_INT_PRINT_DEC";%.6lf;%.6lf;%s;%u",
+ li.filename, li.lineno, bb->count, annotated?1:0,
+ probability/REG_BR_PROB_BASE,
+ e->probability/(dou
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
I'm glad this feature is useful to people. I haven't looked at the normal profiling process though. If anybody would like to port this code to normal profiling, I'm more than pleased to see that happen. Maybe I'll do that myself when I have time, but do not count on that :) On Fri, Jun 27, 2014 at 12:18 PM, Andi Kleen wrote: > Yi Yang writes: > >> Hi, >> >> This patch adds an option. When the option is enabled, GCC will add a >> record about it in an elf section called >> ".gnu.switches.text.branch.annotation" for every branch. > > This would be nice to have even in mainline for the normal profiling. > > -Andi > > -- > a...@linux.intel.com -- Speaking for myself only
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
I refactored the code and added comments. A bug (prematurely breaking
from a loop) was fixed during the refactoring.
(My last mail was wrongly set to HTML instead of plain text. I
apologize for that.)
2014-06-30 Yi Yang
* auto-profile.c (get_locus_information)
(fill_invalid_locus_information, record_branch_prediction_results)
(afdo_calculate_branch_prob, afdo_annotate_cfg): Main comparison and
reporting logic.
* cfg-flags.def (PREDICTED_BY_EXPECT): Add an extra flag representing
an edge's probability is predicted by annotations.
* predict.c (combine_predictions_for_bb): Set up the extra flag on an
edge when appropriate.
* common.opt (fcheck-branch-annotation)
(fcheck-branch-annotation-threshold=): Add an extra GCC option to turn
on report
On Fri, Jun 27, 2014 at 3:20 PM, Xinliang David Li wrote:
> Hi Yi,
>
> 1) please add comments before new functions as documentation -- follow
> the coding style guideline
> 2) missing documenation on the new flags (pointed out by Gerald)
> 3) Please refactor the check code in afdo_calculate_branch_prob into a
> helper function
>
> 4) the change log is not needed for google branches, but if provided,
> the format should follow the style guide (e.g, function name in () ).
>
> David
>
>
> On Fri, Jun 27, 2014 at 11:07 AM, Yi Yang wrote:
>> Hi,
>>
>> This patch adds an option. When the option is enabled, GCC will add a
>> record about it in an elf section called
>> ".gnu.switches.text.branch.annotation" for every branch.
>>
>> gcc/
>>
>> 2014-06-27 Yi Yang
>>
>> * auto-profile.c: Main comparison and reporting logic.
>> * cfg-flags.def: Add an extra flag representing an edge's
>> probability is predicted by annotations.
>> * predict.c: Set up the extra flag on an edge when appropriate.
>> * common.opt: Add an extra GCC option to turn on this report
>> mechanism
diff --git gcc/auto-profile.c gcc/auto-profile.c
index 74d3d1d..d0904ed 100644
--- gcc/auto-profile.c
+++ gcc/auto-profile.c
@@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see
#include "opts.h"/* for in_fnames. */
#include "tree-pass.h" /* for ipa pass. */
#include "cfgloop.h" /* for loop_optimizer_init. */
+#include "md5.h" /* for hashing function names. */
#include "gimple.h"
#include "cgraph.h"
#include "tree-flow.h"
@@ -1367,6 +1368,148 @@ afdo_propagate (void)
}
}
+/* All information parsed from a location_t that will be stored into the ELF
+ section. */
+
+struct locus_information_t {
+ /* File name of the source file containing the branch. */
+ char filename[1024];
+ /* Line number of the branch location. */
+ unsigned lineno;
+ /* Line number of the first line of function definition. */
+ unsigned function_lineno;
+ /* Hash value calculated from function name and length, used to uniquely
+ identify a function across different source versions. */
+ char function_hash[33];
+};
+
+/* Return true iff file and lineno are available for the provided locus.
+ Fill all fields of li with information about locus. */
+
+static bool
+get_locus_information (location_t locus, locus_information_t* li) {
+ if (locus == UNKNOWN_LOCATION || !LOCATION_FILE (locus))
+return false;
+ snprintf (li->filename, 1024, "%s", LOCATION_FILE (locus));
+ li->lineno = LOCATION_LINE (locus);
+
+ tree block = LOCATION_BLOCK (locus);
+ tree function_decl = current_function_decl;
+
+ if (block && TREE_CODE (block) == BLOCK)
+{
+ for (block = BLOCK_SUPERCONTEXT (block);
+ block && (TREE_CODE (block) == BLOCK);
+ block = BLOCK_SUPERCONTEXT (block))
+ {
+ location_t tmp_locus = BLOCK_SOURCE_LOCATION (block);
+ if (LOCATION_LOCUS (tmp_locus) == UNKNOWN_LOCATION)
+ continue;
+
+ tree decl = get_function_decl_from_block (block);
+ function_decl = decl;
+ break;
+ }
+}
+
+ if (!(function_decl && TREE_CODE (function_decl) == FUNCTION_DECL))
+return false;
+ unsigned function_length = 0;
+ function *f = DECL_STRUCT_FUNCTION (function_decl);
+
+ li->function_lineno = LOCATION_LINE (DECL_SOURCE_LOCATION (function_decl));
+
+ if (f)
+{
+ function_length = LOCATION_LINE (f->function_end_locus) -
+ li->function_lineno;
+}
+
+ const char *fn_name = fndecl_name (function_decl);
+ unsigned char md5_result[16];
+
+ md5_ctx ctx;
+
+ md5_init_ctx (&ctx);
+ md5_process_bytes (fn_name, strlen (fn_name), &ctx);
+ md5_process_bytes (&function_length, sizeof (function_length), &ctx);
+ md5_finish_ctx (&ctx, md5_result);
+
+ /* Convert MD5 to hexadecimal
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
Fixed.
Also, I spotted some warnings caused by me using "%lf"s in snprintf().
I changed these to "%f" and tested.
On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen wrote:
> You don't need extra space to store file name in locus_information_t.
> Use pointer instead.
>
> Dehao
>
>
> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang wrote:
>>
>> I refactored the code and added comments. A bug (prematurely breaking
>> from a loop) was fixed during the refactoring.
>>
>> (My last mail was wrongly set to HTML instead of plain text. I
>> apologize for that.)
>>
>> 2014-06-30 Yi Yang
>>
>> * auto-profile.c (get_locus_information)
>> (fill_invalid_locus_information, record_branch_prediction_results)
>> (afdo_calculate_branch_prob, afdo_annotate_cfg): Main comparison and
>> reporting logic.
>> * cfg-flags.def (PREDICTED_BY_EXPECT): Add an extra flag representing
>> an edge's probability is predicted by annotations.
>> * predict.c (combine_predictions_for_bb): Set up the extra flag on an
>> edge when appropriate.
>> * common.opt (fcheck-branch-annotation)
>> (fcheck-branch-annotation-threshold=): Add an extra GCC option to turn
>> on report
>>
>> On Fri, Jun 27, 2014 at 3:20 PM, Xinliang David Li
>> wrote:
>> > Hi Yi,
>> >
>> > 1) please add comments before new functions as documentation -- follow
>> > the coding style guideline
>> > 2) missing documenation on the new flags (pointed out by Gerald)
>> > 3) Please refactor the check code in afdo_calculate_branch_prob into a
>> > helper function
>> >
>> > 4) the change log is not needed for google branches, but if provided,
>> > the format should follow the style guide (e.g, function name in () ).
>> >
>> > David
>> >
>> >
>> > On Fri, Jun 27, 2014 at 11:07 AM, Yi Yang wrote:
>> >> Hi,
>> >>
>> >> This patch adds an option. When the option is enabled, GCC will add a
>> >> record about it in an elf section called
>> >> ".gnu.switches.text.branch.annotation" for every branch.
>> >>
>> >> gcc/
>> >>
>> >> 2014-06-27 Yi Yang
>> >>
>> >> * auto-profile.c: Main comparison and reporting logic.
>> >> * cfg-flags.def: Add an extra flag representing an edge's
>> >> probability is predicted by annotations.
>> >> * predict.c: Set up the extra flag on an edge when appropriate.
>> >> * common.opt: Add an extra GCC option to turn on this report
>> >> mechanism
diff --git gcc/auto-profile.c gcc/auto-profile.c
index 74d3d1d..96cad49 100644
--- gcc/auto-profile.c
+++ gcc/auto-profile.c
@@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see
#include "opts.h"/* for in_fnames. */
#include "tree-pass.h" /* for ipa pass. */
#include "cfgloop.h" /* for loop_optimizer_init. */
+#include "md5.h" /* for hashing function names. */
#include "gimple.h"
#include "cgraph.h"
#include "tree-flow.h"
@@ -1367,6 +1368,148 @@ afdo_propagate (void)
}
}
+/* All information parsed from a location_t that will be stored into the ELF
+ section. */
+
+struct locus_information_t {
+ /* File name of the source file containing the branch. */
+ const char *filename;
+ /* Line number of the branch location. */
+ unsigned lineno;
+ /* Line number of the first line of function definition. */
+ unsigned function_lineno;
+ /* Hash value calculated from function name and length, used to uniquely
+ identify a function across different source versions. */
+ char function_hash[33];
+};
+
+/* Return true iff file and lineno are available for the provided locus.
+ Fill all fields of li with information about locus. */
+
+static bool
+get_locus_information (location_t locus, locus_information_t* li) {
+ if (locus == UNKNOWN_LOCATION || !LOCATION_FILE (locus))
+return false;
+ li->filename = LOCATION_FILE (locus);
+ li->lineno = LOCATION_LINE (locus);
+
+ tree block = LOCATION_BLOCK (locus);
+ tree function_decl = current_function_decl;
+
+ if (block && TREE_CODE (block) == BLOCK)
+{
+ for (block = BLOCK_SUPERCONTEXT (block);
+ block && (TREE_CODE (block) == BLOCK);
+ block = BLOCK_SUPERCONTEXT (block))
+ {
+ location_t tmp_locus = BLOCK_SOURCE_LOCATION (block);
+ if (LOCATION_LOCUS (tmp_locus) == UNKNOWN_LOCATION)
+ continue;
+
+ tree decl = get_function_decl_from_block (bl
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
This is intermediate result, which is meant to be consumed by further post-processing. For this reason I'd prefer to put a number without that percentage sign. I'd just output (int)(probability*1+0.5). Does this look good for you? Or maybe change that to 100 since six digits are more than enough. I don't see a reason to intentionally drop precision though. Note that for the actual probability, the best way to store it is to store the edge count, since the probability is just edge_count/bb_count. But this causes disparity in the formats of the two probabilities. On Mon, Jun 30, 2014 at 2:12 PM, Dehao Chen wrote: > Let's use %d to replace %f (manual conversion, let's do xx%). > > Dehao > > On Mon, Jun 30, 2014 at 2:06 PM, Yi Yang wrote: >> Fixed. >> >> Also, I spotted some warnings caused by me using "%lf"s in snprintf(). >> I changed these to "%f" and tested. >> >> >> On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen wrote: >>> You don't need extra space to store file name in locus_information_t. >>> Use pointer instead. >>> >>> Dehao >>> >>> >>> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang wrote: >>>> >>>> I refactored the code and added comments. A bug (prematurely breaking >>>> from a loop) was fixed during the refactoring. >>>> >>>> (My last mail was wrongly set to HTML instead of plain text. I >>>> apologize for that.) >>>> >>>> 2014-06-30 Yi Yang >>>> >>>> * auto-profile.c (get_locus_information) >>>> (fill_invalid_locus_information, record_branch_prediction_results) >>>> (afdo_calculate_branch_prob, afdo_annotate_cfg): Main comparison and >>>> reporting logic. >>>> * cfg-flags.def (PREDICTED_BY_EXPECT): Add an extra flag representing >>>> an edge's probability is predicted by annotations. >>>> * predict.c (combine_predictions_for_bb): Set up the extra flag on an >>>> edge when appropriate. >>>> * common.opt (fcheck-branch-annotation) >>>> (fcheck-branch-annotation-threshold=): Add an extra GCC option to turn >>>> on report >>>> >>>> On Fri, Jun 27, 2014 at 3:20 PM, Xinliang David Li >>>> wrote: >>>> > Hi Yi, >>>> > >>>> > 1) please add comments before new functions as documentation -- follow >>>> > the coding style guideline >>>> > 2) missing documenation on the new flags (pointed out by Gerald) >>>> > 3) Please refactor the check code in afdo_calculate_branch_prob into a >>>> > helper function >>>> > >>>> > 4) the change log is not needed for google branches, but if provided, >>>> > the format should follow the style guide (e.g, function name in () ). >>>> > >>>> > David >>>> > >>>> > >>>> > On Fri, Jun 27, 2014 at 11:07 AM, Yi Yang wrote: >>>> >> Hi, >>>> >> >>>> >> This patch adds an option. When the option is enabled, GCC will add a >>>> >> record about it in an elf section called >>>> >> ".gnu.switches.text.branch.annotation" for every branch. >>>> >> >>>> >> gcc/ >>>> >> >>>> >> 2014-06-27 Yi Yang >>>> >> >>>> >> * auto-profile.c: Main comparison and reporting logic. >>>> >> * cfg-flags.def: Add an extra flag representing an edge's >>>> >> probability is predicted by annotations. >>>> >> * predict.c: Set up the extra flag on an edge when appropriate. >>>> >> * common.opt: Add an extra GCC option to turn on this report >>>> >> mechanism
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
Fixed. (outputting only the integer percentage) On Mon, Jun 30, 2014 at 2:20 PM, Yi Yang wrote: > This is intermediate result, which is meant to be consumed by further > post-processing. For this reason I'd prefer to put a number without > that percentage sign. > > I'd just output (int)(probability*1+0.5). Does this look good > for you? Or maybe change that to 100 since six digits are more > than enough. I don't see a reason to intentionally drop precision > though. > > Note that for the actual probability, the best way to store it is to > store the edge count, since the probability is just > edge_count/bb_count. But this causes disparity in the formats of the > two probabilities. > > On Mon, Jun 30, 2014 at 2:12 PM, Dehao Chen wrote: >> Let's use %d to replace %f (manual conversion, let's do xx%). >> >> Dehao >> >> On Mon, Jun 30, 2014 at 2:06 PM, Yi Yang wrote: >>> Fixed. >>> >>> Also, I spotted some warnings caused by me using "%lf"s in snprintf(). >>> I changed these to "%f" and tested. >>> >>> >>> On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen wrote: >>>> You don't need extra space to store file name in locus_information_t. >>>> Use pointer instead. >>>> >>>> Dehao >>>> >>>> >>>> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang wrote: >>>>> >>>>> I refactored the code and added comments. A bug (prematurely breaking >>>>> from a loop) was fixed during the refactoring. >>>>> >>>>> (My last mail was wrongly set to HTML instead of plain text. I >>>>> apologize for that.) >>>>> >>>>> 2014-06-30 Yi Yang >>>>> >>>>> * auto-profile.c (get_locus_information) >>>>> (fill_invalid_locus_information, record_branch_prediction_results) >>>>> (afdo_calculate_branch_prob, afdo_annotate_cfg): Main comparison and >>>>> reporting logic. >>>>> * cfg-flags.def (PREDICTED_BY_EXPECT): Add an extra flag representing >>>>> an edge's probability is predicted by annotations. >>>>> * predict.c (combine_predictions_for_bb): Set up the extra flag on an >>>>> edge when appropriate. >>>>> * common.opt (fcheck-branch-annotation) >>>>> (fcheck-branch-annotation-threshold=): Add an extra GCC option to turn >>>>> on report >>>>> >>>>> On Fri, Jun 27, 2014 at 3:20 PM, Xinliang David Li >>>>> wrote: >>>>> > Hi Yi, >>>>> > >>>>> > 1) please add comments before new functions as documentation -- follow >>>>> > the coding style guideline >>>>> > 2) missing documenation on the new flags (pointed out by Gerald) >>>>> > 3) Please refactor the check code in afdo_calculate_branch_prob into a >>>>> > helper function >>>>> > >>>>> > 4) the change log is not needed for google branches, but if provided, >>>>> > the format should follow the style guide (e.g, function name in () ). >>>>> > >>>>> > David >>>>> > >>>>> > >>>>> > On Fri, Jun 27, 2014 at 11:07 AM, Yi Yang wrote: >>>>> >> Hi, >>>>> >> >>>>> >> This patch adds an option. When the option is enabled, GCC will add a >>>>> >> record about it in an elf section called >>>>> >> ".gnu.switches.text.branch.annotation" for every branch. >>>>> >> >>>>> >> gcc/ >>>>> >> >>>>> >> 2014-06-27 Yi Yang >>>>> >> >>>>> >> * auto-profile.c: Main comparison and reporting logic. >>>>> >> * cfg-flags.def: Add an extra flag representing an edge's >>>>> >> probability is predicted by annotations. >>>>> >> * predict.c: Set up the extra flag on an edge when appropriate. >>>>> >> * common.opt: Add an extra GCC option to turn on this report >>>>> >> mechanism diff --git gcc/auto-profile.c gcc/auto-profile.c index 74d3d1d..d8901b9 100644 --- gcc/auto-profile.c +++ gcc/auto-profile.c @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see #include "opts.h"/* for in
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
Instead of storing percentages of the branch probabilities, store them times REG_BR_PROB_BASE. On Mon, Jun 30, 2014 at 3:26 PM, Yi Yang wrote: > Fixed. (outputting only the integer percentage) > > On Mon, Jun 30, 2014 at 2:20 PM, Yi Yang wrote: >> This is intermediate result, which is meant to be consumed by further >> post-processing. For this reason I'd prefer to put a number without >> that percentage sign. >> >> I'd just output (int)(probability*1+0.5). Does this look good >> for you? Or maybe change that to 100 since six digits are more >> than enough. I don't see a reason to intentionally drop precision >> though. >> >> Note that for the actual probability, the best way to store it is to >> store the edge count, since the probability is just >> edge_count/bb_count. But this causes disparity in the formats of the >> two probabilities. >> >> On Mon, Jun 30, 2014 at 2:12 PM, Dehao Chen wrote: >>> Let's use %d to replace %f (manual conversion, let's do xx%). >>> >>> Dehao >>> >>> On Mon, Jun 30, 2014 at 2:06 PM, Yi Yang wrote: >>>> Fixed. >>>> >>>> Also, I spotted some warnings caused by me using "%lf"s in snprintf(). >>>> I changed these to "%f" and tested. >>>> >>>> >>>> On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen wrote: >>>>> You don't need extra space to store file name in locus_information_t. >>>>> Use pointer instead. >>>>> >>>>> Dehao >>>>> >>>>> >>>>> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang wrote: >>>>>> >>>>>> I refactored the code and added comments. A bug (prematurely breaking >>>>>> from a loop) was fixed during the refactoring. >>>>>> >>>>>> (My last mail was wrongly set to HTML instead of plain text. I >>>>>> apologize for that.) >>>>>> >>>>>> 2014-06-30 Yi Yang >>>>>> >>>>>> * auto-profile.c (get_locus_information) >>>>>> (fill_invalid_locus_information, record_branch_prediction_results) >>>>>> (afdo_calculate_branch_prob, afdo_annotate_cfg): Main comparison and >>>>>> reporting logic. >>>>>> * cfg-flags.def (PREDICTED_BY_EXPECT): Add an extra flag representing >>>>>> an edge's probability is predicted by annotations. >>>>>> * predict.c (combine_predictions_for_bb): Set up the extra flag on an >>>>>> edge when appropriate. >>>>>> * common.opt (fcheck-branch-annotation) >>>>>> (fcheck-branch-annotation-threshold=): Add an extra GCC option to >>>>>> turn >>>>>> on report >>>>>> >>>>>> On Fri, Jun 27, 2014 at 3:20 PM, Xinliang David Li >>>>>> wrote: >>>>>> > Hi Yi, >>>>>> > >>>>>> > 1) please add comments before new functions as documentation -- follow >>>>>> > the coding style guideline >>>>>> > 2) missing documenation on the new flags (pointed out by Gerald) >>>>>> > 3) Please refactor the check code in afdo_calculate_branch_prob into a >>>>>> > helper function >>>>>> > >>>>>> > 4) the change log is not needed for google branches, but if provided, >>>>>> > the format should follow the style guide (e.g, function name in () ). >>>>>> > >>>>>> > David >>>>>> > >>>>>> > >>>>>> > On Fri, Jun 27, 2014 at 11:07 AM, Yi Yang wrote: >>>>>> >> Hi, >>>>>> >> >>>>>> >> This patch adds an option. When the option is enabled, GCC will add a >>>>>> >> record about it in an elf section called >>>>>> >> ".gnu.switches.text.branch.annotation" for every branch. >>>>>> >> >>>>>> >> gcc/ >>>>>> >> >>>>>> >> 2014-06-27 Yi Yang >>>>>> >> >>>>>> >> * auto-profile.c: Main comparison and reporting logic. >>>>>> >> * cfg-flags.def: Add an extra flag representing an edge's >>>>>> >> probability is
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
Removed fill_invalid_locus_information. Change the function call to a return statement. On Mon, Jun 30, 2014 at 4:42 PM, Dehao Chen wrote: > There is no need for fill_invalid_locus_information, just initialize > every field to 0, and if it's unknown location, no need to output this > line. > > Dehao > > On Mon, Jun 30, 2014 at 4:26 PM, Yi Yang wrote: >> Instead of storing percentages of the branch probabilities, store them >> times REG_BR_PROB_BASE. >> >> On Mon, Jun 30, 2014 at 3:26 PM, Yi Yang wrote: >>> Fixed. (outputting only the integer percentage) >>> >>> On Mon, Jun 30, 2014 at 2:20 PM, Yi Yang wrote: >>>> This is intermediate result, which is meant to be consumed by further >>>> post-processing. For this reason I'd prefer to put a number without >>>> that percentage sign. >>>> >>>> I'd just output (int)(probability*1+0.5). Does this look good >>>> for you? Or maybe change that to 100 since six digits are more >>>> than enough. I don't see a reason to intentionally drop precision >>>> though. >>>> >>>> Note that for the actual probability, the best way to store it is to >>>> store the edge count, since the probability is just >>>> edge_count/bb_count. But this causes disparity in the formats of the >>>> two probabilities. >>>> >>>> On Mon, Jun 30, 2014 at 2:12 PM, Dehao Chen wrote: >>>>> Let's use %d to replace %f (manual conversion, let's do xx%). >>>>> >>>>> Dehao >>>>> >>>>> On Mon, Jun 30, 2014 at 2:06 PM, Yi Yang wrote: >>>>>> Fixed. >>>>>> >>>>>> Also, I spotted some warnings caused by me using "%lf"s in snprintf(). >>>>>> I changed these to "%f" and tested. >>>>>> >>>>>> >>>>>> On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen wrote: >>>>>>> You don't need extra space to store file name in locus_information_t. >>>>>>> Use pointer instead. >>>>>>> >>>>>>> Dehao >>>>>>> >>>>>>> >>>>>>> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang wrote: >>>>>>>> >>>>>>>> I refactored the code and added comments. A bug (prematurely breaking >>>>>>>> from a loop) was fixed during the refactoring. >>>>>>>> >>>>>>>> (My last mail was wrongly set to HTML instead of plain text. I >>>>>>>> apologize for that.) >>>>>>>> >>>>>>>> 2014-06-30 Yi Yang >>>>>>>> >>>>>>>> * auto-profile.c (get_locus_information) >>>>>>>> (fill_invalid_locus_information, record_branch_prediction_results) >>>>>>>> (afdo_calculate_branch_prob, afdo_annotate_cfg): Main comparison >>>>>>>> and >>>>>>>> reporting logic. >>>>>>>> * cfg-flags.def (PREDICTED_BY_EXPECT): Add an extra flag >>>>>>>> representing >>>>>>>> an edge's probability is predicted by annotations. >>>>>>>> * predict.c (combine_predictions_for_bb): Set up the extra flag on >>>>>>>> an >>>>>>>> edge when appropriate. >>>>>>>> * common.opt (fcheck-branch-annotation) >>>>>>>> (fcheck-branch-annotation-threshold=): Add an extra GCC option to >>>>>>>> turn >>>>>>>> on report >>>>>>>> >>>>>>>> On Fri, Jun 27, 2014 at 3:20 PM, Xinliang David Li >>>>>>>> wrote: >>>>>>>> > Hi Yi, >>>>>>>> > >>>>>>>> > 1) please add comments before new functions as documentation -- >>>>>>>> > follow >>>>>>>> > the coding style guideline >>>>>>>> > 2) missing documenation on the new flags (pointed out by Gerald) >>>>>>>> > 3) Please refactor the check code in afdo_calculate_branch_prob into >>>>>>>> > a >>>>>>>> > helper function >>>>>>>> > >>>>>>>> > 4) the change log
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
Done. On Mon, Jun 30, 2014 at 5:20 PM, Dehao Chen wrote: > For get_locus_information, can you cal get_inline_stack and directly use its > output to get the function name instead? > > Dehao > > > On Mon, Jun 30, 2014 at 4:47 PM, Yi Yang wrote: >> >> Removed fill_invalid_locus_information. Change the function call to a >> return statement. >> >> On Mon, Jun 30, 2014 at 4:42 PM, Dehao Chen wrote: >> > There is no need for fill_invalid_locus_information, just initialize >> > every field to 0, and if it's unknown location, no need to output this >> > line. >> > >> > Dehao >> > >> > On Mon, Jun 30, 2014 at 4:26 PM, Yi Yang wrote: >> >> Instead of storing percentages of the branch probabilities, store them >> >> times REG_BR_PROB_BASE. >> >> >> >> On Mon, Jun 30, 2014 at 3:26 PM, Yi Yang wrote: >> >>> Fixed. (outputting only the integer percentage) >> >>> >> >>> On Mon, Jun 30, 2014 at 2:20 PM, Yi Yang wrote: >> >>>> This is intermediate result, which is meant to be consumed by further >> >>>> post-processing. For this reason I'd prefer to put a number without >> >>>> that percentage sign. >> >>>> >> >>>> I'd just output (int)(probability*1+0.5). Does this look good >> >>>> for you? Or maybe change that to 100 since six digits are more >> >>>> than enough. I don't see a reason to intentionally drop precision >> >>>> though. >> >>>> >> >>>> Note that for the actual probability, the best way to store it is to >> >>>> store the edge count, since the probability is just >> >>>> edge_count/bb_count. But this causes disparity in the formats of the >> >>>> two probabilities. >> >>>> >> >>>> On Mon, Jun 30, 2014 at 2:12 PM, Dehao Chen wrote: >> >>>>> Let's use %d to replace %f (manual conversion, let's do xx%). >> >>>>> >> >>>>> Dehao >> >>>>> >> >>>>> On Mon, Jun 30, 2014 at 2:06 PM, Yi Yang >> >>>>> wrote: >> >>>>>> Fixed. >> >>>>>> >> >>>>>> Also, I spotted some warnings caused by me using "%lf"s in >> >>>>>> snprintf(). >> >>>>>> I changed these to "%f" and tested. >> >>>>>> >> >>>>>> >> >>>>>> On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen >> >>>>>> wrote: >> >>>>>>> You don't need extra space to store file name in >> >>>>>>> locus_information_t. >> >>>>>>> Use pointer instead. >> >>>>>>> >> >>>>>>> Dehao >> >>>>>>> >> >>>>>>> >> >>>>>>> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang >> >>>>>>> wrote: >> >>>>>>>> >> >>>>>>>> I refactored the code and added comments. A bug (prematurely >> >>>>>>>> breaking >> >>>>>>>> from a loop) was fixed during the refactoring. >> >>>>>>>> >> >>>>>>>> (My last mail was wrongly set to HTML instead of plain text. I >> >>>>>>>> apologize for that.) >> >>>>>>>> >> >>>>>>>> 2014-06-30 Yi Yang >> >>>>>>>> >> >>>>>>>> * auto-profile.c (get_locus_information) >> >>>>>>>> (fill_invalid_locus_information, >> >>>>>>>> record_branch_prediction_results) >> >>>>>>>> (afdo_calculate_branch_prob, afdo_annotate_cfg): Main >> >>>>>>>> comparison and >> >>>>>>>> reporting logic. >> >>>>>>>> * cfg-flags.def (PREDICTED_BY_EXPECT): Add an extra flag >> >>>>>>>> representing >> >>>>>>>> an edge's probability is predicted by annotations. >> >>>>>>>> * predict.c (combine_predictions_for_bb): Set up the extra >> >>>>>>>> flag on an &g
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
Fixed a style error (missing a space before a left parenthesis) On Mon, Jun 30, 2014 at 5:41 PM, Yi Yang wrote: > Done. > > On Mon, Jun 30, 2014 at 5:20 PM, Dehao Chen wrote: >> For get_locus_information, can you cal get_inline_stack and directly use its >> output to get the function name instead? >> >> Dehao >> >> >> On Mon, Jun 30, 2014 at 4:47 PM, Yi Yang wrote: >>> >>> Removed fill_invalid_locus_information. Change the function call to a >>> return statement. >>> >>> On Mon, Jun 30, 2014 at 4:42 PM, Dehao Chen wrote: >>> > There is no need for fill_invalid_locus_information, just initialize >>> > every field to 0, and if it's unknown location, no need to output this >>> > line. >>> > >>> > Dehao >>> > >>> > On Mon, Jun 30, 2014 at 4:26 PM, Yi Yang wrote: >>> >> Instead of storing percentages of the branch probabilities, store them >>> >> times REG_BR_PROB_BASE. >>> >> >>> >> On Mon, Jun 30, 2014 at 3:26 PM, Yi Yang wrote: >>> >>> Fixed. (outputting only the integer percentage) >>> >>> >>> >>> On Mon, Jun 30, 2014 at 2:20 PM, Yi Yang wrote: >>> >>>> This is intermediate result, which is meant to be consumed by further >>> >>>> post-processing. For this reason I'd prefer to put a number without >>> >>>> that percentage sign. >>> >>>> >>> >>>> I'd just output (int)(probability*1+0.5). Does this look good >>> >>>> for you? Or maybe change that to 100 since six digits are more >>> >>>> than enough. I don't see a reason to intentionally drop precision >>> >>>> though. >>> >>>> >>> >>>> Note that for the actual probability, the best way to store it is to >>> >>>> store the edge count, since the probability is just >>> >>>> edge_count/bb_count. But this causes disparity in the formats of the >>> >>>> two probabilities. >>> >>>> >>> >>>> On Mon, Jun 30, 2014 at 2:12 PM, Dehao Chen wrote: >>> >>>>> Let's use %d to replace %f (manual conversion, let's do xx%). >>> >>>>> >>> >>>>> Dehao >>> >>>>> >>> >>>>> On Mon, Jun 30, 2014 at 2:06 PM, Yi Yang >>> >>>>> wrote: >>> >>>>>> Fixed. >>> >>>>>> >>> >>>>>> Also, I spotted some warnings caused by me using "%lf"s in >>> >>>>>> snprintf(). >>> >>>>>> I changed these to "%f" and tested. >>> >>>>>> >>> >>>>>> >>> >>>>>> On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen >>> >>>>>> wrote: >>> >>>>>>> You don't need extra space to store file name in >>> >>>>>>> locus_information_t. >>> >>>>>>> Use pointer instead. >>> >>>>>>> >>> >>>>>>> Dehao >>> >>>>>>> >>> >>>>>>> >>> >>>>>>> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang >>> >>>>>>> wrote: >>> >>>>>>>> >>> >>>>>>>> I refactored the code and added comments. A bug (prematurely >>> >>>>>>>> breaking >>> >>>>>>>> from a loop) was fixed during the refactoring. >>> >>>>>>>> >>> >>>>>>>> (My last mail was wrongly set to HTML instead of plain text. I >>> >>>>>>>> apologize for that.) >>> >>>>>>>> >>> >>>>>>>> 2014-06-30 Yi Yang >>> >>>>>>>> >>> >>>>>>>> * auto-profile.c (get_locus_information) >>> >>>>>>>> (fill_invalid_locus_information, >>> >>>>>>>> record_branch_prediction_results) >>> >>>>>>>> (afdo_calculate_branch_prob, afdo_annotate_cfg): Main >>> >>>>>>>> comparison and >>> >>>&g
Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.
Per offline discussion, * do not export function start line number. Instead, hash branch offset and discriminator into the "function_hash" (renamed to just "hash" to clarify) * protect all operations about the new EDGE_PREDICTED_BY_EXPECT flag with flag_check_branch_annotation. On Mon, Jun 30, 2014 at 5:42 PM, Yi Yang wrote: > Fixed a style error (missing a space before a left parenthesis) > > > On Mon, Jun 30, 2014 at 5:41 PM, Yi Yang wrote: >> Done. >> >> On Mon, Jun 30, 2014 at 5:20 PM, Dehao Chen wrote: >>> For get_locus_information, can you cal get_inline_stack and directly use its >>> output to get the function name instead? >>> >>> Dehao >>> >>> >>> On Mon, Jun 30, 2014 at 4:47 PM, Yi Yang wrote: >>>> >>>> Removed fill_invalid_locus_information. Change the function call to a >>>> return statement. >>>> >>>> On Mon, Jun 30, 2014 at 4:42 PM, Dehao Chen wrote: >>>> > There is no need for fill_invalid_locus_information, just initialize >>>> > every field to 0, and if it's unknown location, no need to output this >>>> > line. >>>> > >>>> > Dehao >>>> > >>>> > On Mon, Jun 30, 2014 at 4:26 PM, Yi Yang wrote: >>>> >> Instead of storing percentages of the branch probabilities, store them >>>> >> times REG_BR_PROB_BASE. >>>> >> >>>> >> On Mon, Jun 30, 2014 at 3:26 PM, Yi Yang wrote: >>>> >>> Fixed. (outputting only the integer percentage) >>>> >>> >>>> >>> On Mon, Jun 30, 2014 at 2:20 PM, Yi Yang wrote: >>>> >>>> This is intermediate result, which is meant to be consumed by further >>>> >>>> post-processing. For this reason I'd prefer to put a number without >>>> >>>> that percentage sign. >>>> >>>> >>>> >>>> I'd just output (int)(probability*1+0.5). Does this look good >>>> >>>> for you? Or maybe change that to 100 since six digits are more >>>> >>>> than enough. I don't see a reason to intentionally drop precision >>>> >>>> though. >>>> >>>> >>>> >>>> Note that for the actual probability, the best way to store it is to >>>> >>>> store the edge count, since the probability is just >>>> >>>> edge_count/bb_count. But this causes disparity in the formats of the >>>> >>>> two probabilities. >>>> >>>> >>>> >>>> On Mon, Jun 30, 2014 at 2:12 PM, Dehao Chen wrote: >>>> >>>>> Let's use %d to replace %f (manual conversion, let's do xx%). >>>> >>>>> >>>> >>>>> Dehao >>>> >>>>> >>>> >>>>> On Mon, Jun 30, 2014 at 2:06 PM, Yi Yang >>>> >>>>> wrote: >>>> >>>>>> Fixed. >>>> >>>>>> >>>> >>>>>> Also, I spotted some warnings caused by me using "%lf"s in >>>> >>>>>> snprintf(). >>>> >>>>>> I changed these to "%f" and tested. >>>> >>>>>> >>>> >>>>>> >>>> >>>>>> On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen >>>> >>>>>> wrote: >>>> >>>>>>> You don't need extra space to store file name in >>>> >>>>>>> locus_information_t. >>>> >>>>>>> Use pointer instead. >>>> >>>>>>> >>>> >>>>>>> Dehao >>>> >>>>>>> >>>> >>>>>>> >>>> >>>>>>> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang >>>> >>>>>>> wrote: >>>> >>>>>>>> >>>> >>>>>>>> I refactored the code and added comments. A bug (prematurely >>>> >>>>>>>> breaking >>>> >>>>>>>> from a loop) was fixed during the refactoring. >>>> >>>>>>>> >>>> >>>>>>>> (My last mail was wrongly set to HTML instead of plain text. I >>>
[PATCH] Rewrite edge flag checking statements to prevent problem with new flags
There are a few if statements in cfgrtl.c that are very fragile in the sense that introducing an irrelevant edge flag breaks things. This patch rewrites them to avoid such breakage. -- 2014-07-16 Yi Yang gcc: * cfgrtl.c (rtl_verify_edges, purge_dead_edges): Rewrite certain if statements. --- diff --git gcc/cfgrtl.c gcc/cfgrtl.c index 148c19d..f98ba15 100644 --- gcc/cfgrtl.c +++ gcc/cfgrtl.c @@ -2468,12 +2468,12 @@ rtl_verify_edges (void) err = 1; } - if ((e->flags & ~(EDGE_DFS_BACK -| EDGE_CAN_FALLTHRU -| EDGE_IRREDUCIBLE_LOOP -| EDGE_LOOP_EXIT -| EDGE_CROSSING -| EDGE_PRESERVE)) == 0) + if ((e->flags & (EDGE_ABNORMAL_CALL + | EDGE_SIBCALL + | EDGE_EH + | EDGE_ABNORMAL + | EDGE_FALLTHRU + | EDGE_FAKE)) == 0) n_branch++; if (e->flags & EDGE_ABNORMAL_CALL) @@ -3138,8 +3138,13 @@ purge_dead_edges (basic_block bb) have created the sibcall in the first place. Second, there should of course never have been a fallthru edge. */ gcc_assert (single_succ_p (bb)); - gcc_assert (single_succ_edge (bb)->flags - == (EDGE_SIBCALL | EDGE_ABNORMAL)); + gcc_assert ((single_succ_edge (bb)->flags + & (EDGE_FALLTHRU + | EDGE_SIBCALL + | EDGE_ABNORMAL + | EDGE_EH + | EDGE_ABNORMAL_CALL)) == + (EDGE_SIBCALL | EDGE_ABNORMAL)); return 0; } -- 2.0.0.526.g5318336
Re: FDO and source changes
It's worth noting that merely changing the hash function from crc32 to something that's 64 bit long is enough to make sure collisions does not happen. Maybe it's worth the trouble? On Wed, Jul 23, 2014 at 10:42 AM, Xinliang David Li wrote: > > On Tue, Jul 22, 2014 at 8:14 PM, Jeff Law wrote: > > On 07/16/14 14:32, Xinliang David Li wrote: > >> > >> Instrumentation based FDO is designed to work when the source files > >> that are used to generate the instr binary match exactly with the > >> sources in profile-use compile. It is known historically that using > >> stale profile (due to source changes, not gcda format change) can lead > >> to lots of mismatch warnings and even worse -- compiler ICEs. This is > >> due to two reasons: > >> 1) the profile lookup for each function is based on funcdef_no which > >> can change when function definition order is changed or new functions > >> are inserted in the middle of a source > >> 2) the indirect call target id may change due to source changes: > >> before GCC4.9, the id uses cgraph uid which is as bad as funcdef_no. > >> Attributing wrong IC target to the indirect call site is the main > >> cause of compiler ICE (we have signature match check, but bad target > >> can leak through result in problem later). Starting from gcc49, the > >> indirect target profiling uses profile_id which is stable for public > >> functions. > >> > >> This patch introduces a new parameter for FDO to determine whether to > >> use internal id or assembler name based external id for profile > >> lookup. When the external id is used, GCC FDO will become very > >> tolerant to simple source changes. > >> > >> Note that autoFDO solves this problem but it is currently limited to > >> Intel platforms with LBR support. > >> > >> (Tested with SPEC, SPEC06 and large internal benchmarks. No performance > >> impact). > >> > >> Ok for trunk? > > > > Is there a good reason why we would want to ever use the internal id? Is it > > because the internal id shows up in the profile data for preexisting files? > > I don't think existing profile data matter. > > For perfect fresh profile, using external id has the chance of > collision. I have tested with a C++ symbol file with about 750k unique > symbol names, using crc32 based id yields 71 collisions --- the rate > is ~0.009%. > > > > > Given that we need both, why is this a param vs a regular -f option? > > Shouldn't the default be to use the external id? > > > > I am open to both. I have not seen evidence that id collision causes > trouble even though in theory it can. > > thanks, > > David > > > > BTW, thanks for working on this. I've certainly got customers that want to > > see the FDO data be more tolerant of changes. > > > > Heff > >
Re: [PATCH] Rewrite edge flag checking statements to prevent problem with new flags
I agree with you that rtl_verify_edges() shouldn't be changed. While I am not sure about purge_dead_edges() since it is not something one looks at when bringing a new flag. But it hasn't caused problems so far, so I can't necessitate changing it either... On Tue, Jul 22, 2014 at 8:16 PM, Jeff Law wrote: > On 07/16/14 12:41, Yi Yang wrote: >> >> There are a few if statements in cfgrtl.c that are very fragile in the >> sense that introducing an irrelevant edge flag breaks things. >> >> This patch rewrites them to avoid such breakage. >> >> -- >> >> 2014-07-16 Yi Yang >> >> gcc: >> * cfgrtl.c (rtl_verify_edges, purge_dead_edges): Rewrite certain >> if statements. > > One could easily argue that 'fragile' in this context is what we actually > want. Ie, if someone introduces a new edge flag, they ought to be looking > at how that impacts verification. > > I strongly prefer to have developers explicitly handle any new edge flags in > the verification code. If they are truly irrelevant for verification, it's > easy enough to filter them out in the appropriate verification routines. > > Jeff
