Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
To avoid the unused new discriminator value, I added a map
found_call_this_line to track whether a call is the first call in a
source line seen when assigning discriminators. For the first call in
a source line, its discriminator is 0. For the following calls in the
same source line, a new discriminator will be used everytime. The new
patch is attached. Internal perf test and regression test are ok. Is
it ok for google-4_9?
This seems overly complex to me. I'd think all you need to do is add a
bit to locus_discrim_map (stealing a bit from discriminator ought to
be fine) that indicates whether the next call should increment the
discriminator or not. Something like this:
increase_discriminator_for_locus (location_t locus, bool return_next)
{
...
if (return_next || (*slot)-needs_increment)
{
(*slot)-discriminator++;
(*slot)-needs_increment = false;
}
else
(*slot)-needs_increment = true;
return (*slot)-discriminator;
}
-cary
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
Thanks, that is ellegant. Will paste a new patch in this way soon.
Wei.
On Fri, Aug 29, 2014 at 10:11 AM, Cary Coutant ccout...@google.com wrote:
To avoid the unused new discriminator value, I added a map
found_call_this_line to track whether a call is the first call in a
source line seen when assigning discriminators. For the first call in
a source line, its discriminator is 0. For the following calls in the
same source line, a new discriminator will be used everytime. The new
patch is attached. Internal perf test and regression test are ok. Is
it ok for google-4_9?
This seems overly complex to me. I'd think all you need to do is add a
bit to locus_discrim_map (stealing a bit from discriminator ought to
be fine) that indicates whether the next call should increment the
discriminator or not. Something like this:
increase_discriminator_for_locus (location_t locus, bool return_next)
{
...
if (return_next || (*slot)-needs_increment)
{
(*slot)-discriminator++;
(*slot)-needs_increment = false;
}
else
(*slot)-needs_increment = true;
return (*slot)-discriminator;
}
-cary
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
On Fri, Aug 29, 2014 at 10:11 AM, Cary Coutant ccout...@google.com wrote:
To avoid the unused new discriminator value, I added a map
found_call_this_line to track whether a call is the first call in a
source line seen when assigning discriminators. For the first call in
a source line, its discriminator is 0. For the following calls in the
same source line, a new discriminator will be used everytime. The new
patch is attached. Internal perf test and regression test are ok. Is
it ok for google-4_9?
This seems overly complex to me. I'd think all you need to do is add a
bit to locus_discrim_map (stealing a bit from discriminator ought to
be fine) that indicates whether the next call should increment the
discriminator or not. Something like this:
increase_discriminator_for_locus (location_t locus, bool return_next)
{
...
if (return_next || (*slot)-needs_increment)
{
(*slot)-discriminator++;
(*slot)-needs_increment = false;
}
else
(*slot)-needs_increment = true;
return (*slot)-discriminator;
}
-cary
Here is the new patch (attached). Regression test passes. Cary, is it ok?
Thanks,
Wei.
ChangeLog:
2014-08-29 Wei Mi w...@google.com
* tree-cfg.c (struct locus_discrim_map): New field needs_increment.
(next_discriminator_for_locus): Increase discriminator only when
return_next or needs_increment are true.
(assign_discriminator): Add an actual for next_discriminator_for_locus.
(assign_discriminators): Assign different discriminators for calls
belonging to the same source line.
Index: tree-cfg.c
===
--- tree-cfg.c (revision 213402)
+++ tree-cfg.c (working copy)
@@ -112,7 +112,14 @@ static struct cfg_stats_d cfg_stats;
struct locus_discrim_map
{
location_t locus;
- int discriminator;
+ /* Different calls belonging to the same source line will be assigned
+ different discriminators. But we want to keep the discriminator of
+ the first call in the same source line to be 0, in order to reduce
+ the .debug_line section size. needs_increment is used for this
+ purpose. It is initialized as false and will be set to true after
+ the first call is seen. */
+ bool needs_increment:1;
+ int discriminator:31;
};
/* Hashtable helpers. */
@@ -914,10 +921,15 @@ make_edges (void)
/* Find the next available discriminator value for LOCUS. The
discriminator distinguishes among several basic blocks that
share a common locus, allowing for more accurate sample-based
- profiling. */
+ profiling. If RETURN_NEXT is true, return the next discriminator
+ anyway. If RETURN_NEXT is not true, we may not increase the
+ discriminator if locus_discrim_map::needs_increment is false,
+ which is used when the stmt is the first call stmt in current
+ source line. locus_discrim_map::needs_increment will be set to
+ true after the first call is seen. */
static int
-next_discriminator_for_locus (location_t locus)
+next_discriminator_for_locus (location_t locus, bool return_next)
{
struct locus_discrim_map item;
struct locus_discrim_map **slot;
@@ -932,9 +944,13 @@ next_discriminator_for_locus (location_t
*slot = XNEW (struct locus_discrim_map);
gcc_assert (*slot);
(*slot)-locus = locus;
+ (*slot)-needs_increment = false;
(*slot)-discriminator = 0;
}
- (*slot)-discriminator++;
+ if (return_next || (*slot)-needs_increment)
+(*slot)-discriminator++;
+ else
+(*slot)-needs_increment = true;
return (*slot)-discriminator;
}
@@ -974,7 +990,7 @@ assign_discriminator (location_t locus,
if (locus == UNKNOWN_LOCATION)
return;
- discriminator = next_discriminator_for_locus (locus);
+ discriminator = next_discriminator_for_locus (locus, true);
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
@@ -1009,23 +1025,13 @@ assign_discriminators (void)
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
gimple stmt = gsi_stmt (gsi);
- if (curr_locus == UNKNOWN_LOCATION)
- {
+ if (gimple_code (stmt) == GIMPLE_CALL)
+{
curr_locus = gimple_location (stmt);
- }
- else if (!same_line_p (curr_locus, gimple_location (stmt)))
- {
- curr_locus = gimple_location (stmt);
- curr_discr = 0;
- }
- else if (curr_discr != 0)
- {
+ curr_discr = next_discriminator_for_locus (curr_locus, false);
gimple_set_location (stmt, location_with_discriminator (
- gimple_location (stmt), curr_discr));
+ curr_locus, curr_discr));
}
- /* Allocate a new discriminator for CALL stmt. */
- if (gimple_code (stmt) == GIMPLE_CALL)
- curr_discr = next_discriminator_for_locus (curr_locus);
}
if (locus
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
2014-08-29 Wei Mi w...@google.com
* tree-cfg.c (struct locus_discrim_map): New field needs_increment.
(next_discriminator_for_locus): Increase discriminator only when
return_next or needs_increment are true.
(assign_discriminator): Add an actual for next_discriminator_for_locus.
(assign_discriminators): Assign different discriminators for calls
belonging to the same source line.
OK for google/gcc-4_9 branch. Thanks!
-cary
On Fri, Aug 29, 2014 at 1:59 PM, Wei Mi w...@google.com wrote:
On Fri, Aug 29, 2014 at 10:11 AM, Cary Coutant ccout...@google.com wrote:
To avoid the unused new discriminator value, I added a map
found_call_this_line to track whether a call is the first call in a
source line seen when assigning discriminators. For the first call in
a source line, its discriminator is 0. For the following calls in the
same source line, a new discriminator will be used everytime. The new
patch is attached. Internal perf test and regression test are ok. Is
it ok for google-4_9?
This seems overly complex to me. I'd think all you need to do is add a
bit to locus_discrim_map (stealing a bit from discriminator ought to
be fine) that indicates whether the next call should increment the
discriminator or not. Something like this:
increase_discriminator_for_locus (location_t locus, bool return_next)
{
...
if (return_next || (*slot)-needs_increment)
{
(*slot)-discriminator++;
(*slot)-needs_increment = false;
}
else
(*slot)-needs_increment = true;
return (*slot)-discriminator;
}
-cary
Here is the new patch (attached). Regression test passes. Cary, is it ok?
Thanks,
Wei.
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
Hi Cary,
Is the new patch ok for google-4_9?
Thanks,
Wei.
On Sun, Aug 24, 2014 at 8:53 PM, Wei Mi w...@google.com wrote:
To avoid the unused new discriminator value, I added a map
found_call_this_line to track whether a call is the first call in a
source line seen when assigning discriminators. For the first call in
a source line, its discriminator is 0. For the following calls in the
same source line, a new discriminator will be used everytime. The new
patch is attached. Internal perf test and regression test are ok. Is
it ok for google-4_9?
Thanks,
Wei.
On Thu, Aug 7, 2014 at 2:10 PM, Wei Mi w...@google.com wrote:
Yes, that is intentional. It is to avoid assiging a discriminator for
the first call in the group of calls with the same source lineno.
Starting from the second call in the group, it will get a different
discriminator with previous call in the same group.
Thanks,
Wei.
On Thu, Aug 7, 2014 at 12:17 PM, Cary Coutant ccout...@google.com wrote:
static int
-next_discriminator_for_locus (location_t locus)
+increase_discriminator_for_locus (location_t locus, bool return_next)
{
struct locus_discrim_map item;
struct locus_discrim_map **slot;
@@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
(*slot)-locus = locus;
(*slot)-discriminator = 0;
}
+
(*slot)-discriminator++;
- return (*slot)-discriminator;
+ return return_next ? (*slot)-discriminator
+: (*slot)-discriminator - 1;
}
Won't this have the effect of sometimes incrementing the next
available discriminator without actually using the new value? That is,
if you call it once with return_next == false, and then with
return_next == true.
-cary
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
To avoid the unused new discriminator value, I added a map
found_call_this_line to track whether a call is the first call in a
source line seen when assigning discriminators. For the first call in
a source line, its discriminator is 0. For the following calls in the
same source line, a new discriminator will be used everytime. The new
patch is attached. Internal perf test and regression test are ok. Is
it ok for google-4_9?
Thanks,
Wei.
On Thu, Aug 7, 2014 at 2:10 PM, Wei Mi w...@google.com wrote:
Yes, that is intentional. It is to avoid assiging a discriminator for
the first call in the group of calls with the same source lineno.
Starting from the second call in the group, it will get a different
discriminator with previous call in the same group.
Thanks,
Wei.
On Thu, Aug 7, 2014 at 12:17 PM, Cary Coutant ccout...@google.com wrote:
static int
-next_discriminator_for_locus (location_t locus)
+increase_discriminator_for_locus (location_t locus, bool return_next)
{
struct locus_discrim_map item;
struct locus_discrim_map **slot;
@@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
(*slot)-locus = locus;
(*slot)-discriminator = 0;
}
+
(*slot)-discriminator++;
- return (*slot)-discriminator;
+ return return_next ? (*slot)-discriminator
+: (*slot)-discriminator - 1;
}
Won't this have the effect of sometimes incrementing the next
available discriminator without actually using the new value? That is,
if you call it once with return_next == false, and then with
return_next == true.
-cary
ChangeLog:
2014-08-24 Wei Mi w...@google.com
* tree-cfg.c (assign_discriminators): Assign different discriminators
for calls belonging to the same source line.
Index: tree-cfg.c
===
--- tree-cfg.c (revision 213402)
+++ tree-cfg.c (working copy)
@@ -992,7 +992,13 @@ static void
assign_discriminators (void)
{
basic_block bb;
+ /* If there is a location saved in the hash_table, it means that we
+ already found a call in the source line before. For the calls which
+ are not the first call found in the same source line, we don't assign
+ new discriminator for it, so that .debug_line section will be smaller. */
+ hash_table locus_discrim_hasher found_call_this_line;
+ found_call_this_line.create (13);
FOR_EACH_BB_FN (bb, cfun)
{
edge e;
@@ -1009,23 +1015,31 @@ assign_discriminators (void)
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
gimple stmt = gsi_stmt (gsi);
- if (curr_locus == UNKNOWN_LOCATION)
- {
- curr_locus = gimple_location (stmt);
- }
- else if (!same_line_p (curr_locus, gimple_location (stmt)))
+ if (gimple_code (stmt) == GIMPLE_CALL)
{
+ struct locus_discrim_map item;
+ struct locus_discrim_map **slot;
+
curr_locus = gimple_location (stmt);
- curr_discr = 0;
- }
- else if (curr_discr != 0)
- {
- gimple_set_location (stmt, location_with_discriminator (
- gimple_location (stmt), curr_discr));
+ item.locus = curr_locus;
+ item.discriminator = 0;
+ slot = found_call_this_line.find_slot (item, INSERT);
+ /* If the current call is not the first call seen in curr_locus,
+assign the next discriminator to it, else keep its
discriminator
+unchanged. */
+ if (*slot != HTAB_EMPTY_ENTRY)
+ {
+ curr_discr = next_discriminator_for_locus (curr_locus);
+ gimple_set_location (stmt, location_with_discriminator (
+ gimple_location (stmt), curr_discr));
+ }
+ else
+ {
+ *slot = XNEW (struct locus_discrim_map);
+ (*slot)-locus = curr_locus;
+ (*slot)-discriminator = 0;
+ }
}
- /* Allocate a new discriminator for CALL stmt. */
- if (gimple_code (stmt) == GIMPLE_CALL)
- curr_discr = next_discriminator_for_locus (curr_locus);
}
if (locus == UNKNOWN_LOCATION)
@@ -1047,6 +1061,7 @@ assign_discriminators (void)
}
}
}
+ found_call_this_line.dispose ();
}
/* Create the edges for a GIMPLE_COND starting at block BB. */
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
static int
-next_discriminator_for_locus (location_t locus)
+increase_discriminator_for_locus (location_t locus, bool return_next)
{
struct locus_discrim_map item;
struct locus_discrim_map **slot;
@@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
(*slot)-locus = locus;
(*slot)-discriminator = 0;
}
+
(*slot)-discriminator++;
- return (*slot)-discriminator;
+ return return_next ? (*slot)-discriminator
+: (*slot)-discriminator - 1;
}
Won't this have the effect of sometimes incrementing the next
available discriminator without actually using the new value? That is,
if you call it once with return_next == false, and then with
return_next == true.
-cary
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
Yes, that is intentional. It is to avoid assiging a discriminator for
the first call in the group of calls with the same source lineno.
Starting from the second call in the group, it will get a different
discriminator with previous call in the same group.
Thanks,
Wei.
On Thu, Aug 7, 2014 at 12:17 PM, Cary Coutant ccout...@google.com wrote:
static int
-next_discriminator_for_locus (location_t locus)
+increase_discriminator_for_locus (location_t locus, bool return_next)
{
struct locus_discrim_map item;
struct locus_discrim_map **slot;
@@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
(*slot)-locus = locus;
(*slot)-discriminator = 0;
}
+
(*slot)-discriminator++;
- return (*slot)-discriminator;
+ return return_next ? (*slot)-discriminator
+: (*slot)-discriminator - 1;
}
Won't this have the effect of sometimes incrementing the next
available discriminator without actually using the new value? That is,
if you call it once with return_next == false, and then with
return_next == true.
-cary
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
No, it is not. This IR is dumped before early inline -- just after
pass_build_cfg. The line number of the deconstructor is marked
according to where its constructor is located, instead of where it is
inserted. This is also problematic.
Wei.
On Thu, Aug 7, 2014 at 12:11 PM, Xinliang David Li davi...@google.com wrote:
Is this
[1.cc : 179:64] Reader::~Reader (version);
from an inline instance?
David
On Wed, Aug 6, 2014 at 10:18 AM, Wei Mi w...@google.com wrote:
We saw bb like this in the IR dump after pass_build_cfg:
bb 21:
[1.cc : 205:45] D.332088 = table-_vptr.Table;
[1.cc : 205:45] D.332134 = D.332088 + 104;
[1.cc : 205:45] D.332135 = [1.cc : 205] *D.332134;
[1.cc : 205:45] D.332092 = [1.cc : 205] this-cp_stream_;
[1.cc : 205:46] OBJ_TYPE_REF(D.332135;(struct Table)table-13) (table,
cp_arg, D.332092); // indirect call
[1.cc : 179:64] Reader::~Reader (version);
[1.cc : 205:46] Switcher::~Switcher (tcswr);
The indirect call above has the same source lineno with Switcher::~Switcher
(tcswr);, but they have no discriminator so they cannot be discriminated
in autofdo. This causes the problem that autofdo mistakenly regards
Switcher::~Switcher (tcswr); as a target of the indirect call above, and
makes a wrong promotion.
The existing code has the logic to assign different discriminators to calls
with the same lineno, but it only works when the lineno in a bb is
monotonical. In this case, there is another stmt with lineno 179 between the
indirect call and Switcher::~Switcher (tcswr); (both with lineno 205), so
existing code will not assign different discriminators for them.
The patch is to assign discriminators for calls with the same lineno anyway.
regression test is going. internal perf test for autofdo shows a little
improvement. Ok for google-4_9 if regression pass?
Thanks,
Wei.
ChangeLog:
2014-08-06 Wei Mi w...@google.com
* tree-cfg.c (increase_discriminator_for_locus): It was
next_discriminator_for_locus. Add a param return_next.
(next_discriminator_for_locus): Renamed.
(assign_discriminator): Use the renamed func.
(assign_discriminators): Assign different discriminators
for calls with the same lineno.
Index: tree-cfg.c
===
--- tree-cfg.c (revision 213402)
+++ tree-cfg.c (working copy)
@@ -914,10 +914,12 @@ make_edges (void)
/* Find the next available discriminator value for LOCUS. The
discriminator distinguishes among several basic blocks that
share a common locus, allowing for more accurate sample-based
- profiling. */
+ profiling. If RETURN_NEXT is true, return the discriminator
+ value after the increase, else return the discriminator value
+ before the increase. */
static int
-next_discriminator_for_locus (location_t locus)
+increase_discriminator_for_locus (location_t locus, bool return_next)
{
struct locus_discrim_map item;
struct locus_discrim_map **slot;
@@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
(*slot)-locus = locus;
(*slot)-discriminator = 0;
}
+
(*slot)-discriminator++;
- return (*slot)-discriminator;
+ return return_next ? (*slot)-discriminator
+: (*slot)-discriminator - 1;
}
/* Return TRUE if LOCUS1 and LOCUS2 refer to the same source line. */
@@ -974,7 +978,7 @@ assign_discriminator (location_t locus,
if (locus == UNKNOWN_LOCATION)
return;
- discriminator = next_discriminator_for_locus (locus);
+ discriminator = increase_discriminator_for_locus (locus, true);
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
@@ -1009,23 +1013,16 @@ assign_discriminators (void)
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
gimple stmt = gsi_stmt (gsi);
- if (curr_locus == UNKNOWN_LOCATION)
- {
- curr_locus = gimple_location (stmt);
- }
- else if (!same_line_p (curr_locus, gimple_location (stmt)))
+ if (gimple_code (stmt) == GIMPLE_CALL)
{
curr_locus = gimple_location (stmt);
- curr_discr = 0;
- }
- else if (curr_discr != 0)
- {
- gimple_set_location (stmt, location_with_discriminator (
- gimple_location (stmt), curr_discr));
+ /* return the current discriminator first, then increase the
+discriminator for next call. */
+ curr_discr = increase_discriminator_for_locus (curr_locus,
false);
+ if (curr_discr != 0)
+ gimple_set_location (stmt, location_with_discriminator (
+ gimple_location (stmt), curr_discr));
}
- /* Allocate a new discriminator for CALL stmt. */
- if (gimple_code (stmt) == GIMPLE_CALL)
- curr_discr =
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
On Thu, Aug 7, 2014 at 2:20 PM, Wei Mi w...@google.com wrote:
No, it is not. This IR is dumped before early inline -- just after
pass_build_cfg. The line number of the deconstructor is marked
according to where its constructor is located,
The definition location or the invocation location?
David
instead of where it is
inserted. This is also problematic.
Wei.
On Thu, Aug 7, 2014 at 12:11 PM, Xinliang David Li davi...@google.com wrote:
Is this
[1.cc : 179:64] Reader::~Reader (version);
from an inline instance?
David
On Wed, Aug 6, 2014 at 10:18 AM, Wei Mi w...@google.com wrote:
We saw bb like this in the IR dump after pass_build_cfg:
bb 21:
[1.cc : 205:45] D.332088 = table-_vptr.Table;
[1.cc : 205:45] D.332134 = D.332088 + 104;
[1.cc : 205:45] D.332135 = [1.cc : 205] *D.332134;
[1.cc : 205:45] D.332092 = [1.cc : 205] this-cp_stream_;
[1.cc : 205:46] OBJ_TYPE_REF(D.332135;(struct Table)table-13) (table,
cp_arg, D.332092); // indirect call
[1.cc : 179:64] Reader::~Reader (version);
[1.cc : 205:46] Switcher::~Switcher (tcswr);
The indirect call above has the same source lineno with Switcher::~Switcher
(tcswr);, but they have no discriminator so they cannot be discriminated
in autofdo. This causes the problem that autofdo mistakenly regards
Switcher::~Switcher (tcswr); as a target of the indirect call above, and
makes a wrong promotion.
The existing code has the logic to assign different discriminators to calls
with the same lineno, but it only works when the lineno in a bb is
monotonical. In this case, there is another stmt with lineno 179 between the
indirect call and Switcher::~Switcher (tcswr); (both with lineno 205), so
existing code will not assign different discriminators for them.
The patch is to assign discriminators for calls with the same lineno anyway.
regression test is going. internal perf test for autofdo shows a little
improvement. Ok for google-4_9 if regression pass?
Thanks,
Wei.
ChangeLog:
2014-08-06 Wei Mi w...@google.com
* tree-cfg.c (increase_discriminator_for_locus): It was
next_discriminator_for_locus. Add a param return_next.
(next_discriminator_for_locus): Renamed.
(assign_discriminator): Use the renamed func.
(assign_discriminators): Assign different discriminators
for calls with the same lineno.
Index: tree-cfg.c
===
--- tree-cfg.c (revision 213402)
+++ tree-cfg.c (working copy)
@@ -914,10 +914,12 @@ make_edges (void)
/* Find the next available discriminator value for LOCUS. The
discriminator distinguishes among several basic blocks that
share a common locus, allowing for more accurate sample-based
- profiling. */
+ profiling. If RETURN_NEXT is true, return the discriminator
+ value after the increase, else return the discriminator value
+ before the increase. */
static int
-next_discriminator_for_locus (location_t locus)
+increase_discriminator_for_locus (location_t locus, bool return_next)
{
struct locus_discrim_map item;
struct locus_discrim_map **slot;
@@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
(*slot)-locus = locus;
(*slot)-discriminator = 0;
}
+
(*slot)-discriminator++;
- return (*slot)-discriminator;
+ return return_next ? (*slot)-discriminator
+: (*slot)-discriminator - 1;
}
/* Return TRUE if LOCUS1 and LOCUS2 refer to the same source line. */
@@ -974,7 +978,7 @@ assign_discriminator (location_t locus,
if (locus == UNKNOWN_LOCATION)
return;
- discriminator = next_discriminator_for_locus (locus);
+ discriminator = increase_discriminator_for_locus (locus, true);
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
@@ -1009,23 +1013,16 @@ assign_discriminators (void)
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
gimple stmt = gsi_stmt (gsi);
- if (curr_locus == UNKNOWN_LOCATION)
- {
- curr_locus = gimple_location (stmt);
- }
- else if (!same_line_p (curr_locus, gimple_location (stmt)))
+ if (gimple_code (stmt) == GIMPLE_CALL)
{
curr_locus = gimple_location (stmt);
- curr_discr = 0;
- }
- else if (curr_discr != 0)
- {
- gimple_set_location (stmt, location_with_discriminator (
- gimple_location (stmt), curr_discr));
+ /* return the current discriminator first, then increase the
+discriminator for next call. */
+ curr_discr = increase_discriminator_for_locus (curr_locus,
false);
+ if (curr_discr != 0)
+ gimple_set_location (stmt, location_with_discriminator (
+ gimple_location (stmt), curr_discr));
}
- /*
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
On Thu, Aug 7, 2014 at 2:40 PM, Xinliang David Li davi...@google.com wrote: On Thu, Aug 7, 2014 at 2:20 PM, Wei Mi w...@google.com wrote: No, it is not. This IR is dumped before early inline -- just after pass_build_cfg. The line number of the deconstructor is marked according to where its constructor is located, The definition location or the invocation location? David The definition location and the invocation location are the same source line for the case. Wei.
Fwd: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
(Sorry if you received the mail twice because it was not plain text at
first and was rejected by @sourceware.org)
We saw bb like this in the IR dump after pass_build_cfg:
bb 21:
[1.cc : 205:45] D.332088 = table-_vptr.Table;
[1.cc : 205:45] D.332134 = D.332088 + 104;
[1.cc : 205:45] D.332135 = [1.cc : 205] *D.332134;
[1.cc : 205:45] D.332092 = [1.cc : 205] this-cp_stream_;
[1.cc : 205:46] OBJ_TYPE_REF(D.332135;(struct Table)table-13)
(table, cp_arg, D.332092); // indirect call
[1.cc : 179:64] Reader::~Reader (version);
[1.cc : 205:46] Switcher::~Switcher (tcswr);
The indirect call above has the same source lineno with
Switcher::~Switcher (tcswr);, but they have no discriminator so
they cannot be discriminated in autofdo. This causes the problem that
autofdo mistakenly regards Switcher::~Switcher (tcswr); as a target
of the indirect call above, and makes a wrong promotion.
The existing code has the logic to assign different discriminators to
calls with the same lineno, but it only works when the lineno in a bb
is monotonical. In this case, there is another stmt with lineno 179
between the indirect call and Switcher::~Switcher (tcswr); (both
with lineno 205), so existing code will not assign different
discriminators for them.
The patch is to assign discriminators for calls with the same lineno anyway.
regression test is going. internal perf test for autofdo shows a
little improvement. Ok for google-4_9 if regression pass?
Thanks,
Wei.
ChangeLog:
2014-08-06 Wei Mi w...@google.com
* tree-cfg.c (increase_discriminator_for_locus): It was
next_discriminator_for_locus. Add a param return_next.
(next_discriminator_for_locus): Renamed.
(assign_discriminator): Use the renamed func.
(assign_discriminators): Assign different discriminators
for calls with the same lineno.
Index: tree-cfg.c
===
--- tree-cfg.c (revision 213402)
+++ tree-cfg.c (working copy)
@@ -914,10 +914,12 @@ make_edges (void)
/* Find the next available discriminator value for LOCUS. The
discriminator distinguishes among several basic blocks that
share a common locus, allowing for more accurate sample-based
- profiling. */
+ profiling. If RETURN_NEXT is true, return the discriminator
+ value after the increase, else return the discriminator value
+ before the increase. */
static int
-next_discriminator_for_locus (location_t locus)
+increase_discriminator_for_locus (location_t locus, bool return_next)
{
struct locus_discrim_map item;
struct locus_discrim_map **slot;
@@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
(*slot)-locus = locus;
(*slot)-discriminator = 0;
}
+
(*slot)-discriminator++;
- return (*slot)-discriminator;
+ return return_next ? (*slot)-discriminator
+: (*slot)-discriminator - 1;
}
/* Return TRUE if LOCUS1 and LOCUS2 refer to the same source line. */
@@ -974,7 +978,7 @@ assign_discriminator (location_t locus,
if (locus == UNKNOWN_LOCATION)
return;
- discriminator = next_discriminator_for_locus (locus);
+ discriminator = increase_discriminator_for_locus (locus, true);
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
@@ -1009,23 +1013,16 @@ assign_discriminators (void)
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
gimple stmt = gsi_stmt (gsi);
- if (curr_locus == UNKNOWN_LOCATION)
- {
- curr_locus = gimple_location (stmt);
- }
- else if (!same_line_p (curr_locus, gimple_location (stmt)))
+ if (gimple_code (stmt) == GIMPLE_CALL)
{
curr_locus = gimple_location (stmt);
- curr_discr = 0;
- }
- else if (curr_discr != 0)
- {
- gimple_set_location (stmt, location_with_discriminator (
- gimple_location (stmt), curr_discr));
+ /* return the current discriminator first, then increase the
+discriminator for next call. */
+ curr_discr = increase_discriminator_for_locus (curr_locus, false);
+ if (curr_discr != 0)
+ gimple_set_location (stmt, location_with_discriminator (
+ gimple_location (stmt), curr_discr));
}
- /* Allocate a new discriminator for CALL stmt. */
- if (gimple_code (stmt) == GIMPLE_CALL)
- curr_discr = next_discriminator_for_locus (curr_locus);
}
if (locus == UNKNOWN_LOCATION)
