diff mbox

Fix incorrect discriminator assignment.

Message ID CAO2gOZVtO8Ux9HdCaMuSSzS8Rxk2usyVDBo+ba9_ehT1POyF0w@mail.gmail.com
State New
Headers show

Commit Message

Dehao Chen May 15, 2013, 4:20 p.m. UTC 
This patch fixes a common case where one line is distributed to 3 BBs,
but only 1 discriminator is assigned.

Bootstrapped and passed gcc regression test.

OK for trunk?

Thanks,
Dehao

gcc/ChangeLog:

* tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
(locus_descrim_hasher::equal): Likewise.
(next_discriminator_for_locus): Likewise.
(assign_discriminator): Add return value.
(make_edges): Assign more discriminator if necessary.
(make_cond_expr_edges): Likewise.
(make_goto_expr_edges): Likewise.

gcc/testsuite/ChangeLog:

* gcc.dg/debug/dwarf2/discriminator.c: New Test.

Comments

Cary Coutant May 15, 2013, 4:50 p.m. UTC | #1 
> gcc/ChangeLog:
>
> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
> (locus_descrim_hasher::equal): Likewise.
> (next_discriminator_for_locus): Likewise.
> (assign_discriminator): Add return value.
> (make_edges): Assign more discriminator if necessary.
> (make_cond_expr_edges): Likewise.
> (make_goto_expr_edges): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/debug/dwarf2/discriminator.c: New Test.

Looks OK to me, but I can't approve patches for tree-cfg.c.

Two comments:

(1) In the C++ conversion, it looks like someone misspelled "discrim"
in "locus_descrim_hasher".

(2) Is this worth fixing in trunk when we'll eventually switch to a
per-instruction discriminator?

-cary
Dehao Chen May 15, 2013, 5:28 p.m. UTC | #2 
On Wed, May 15, 2013 at 9:50 AM, Cary Coutant <ccoutant@google.com> wrote:
>> gcc/ChangeLog:
>>
>> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
>> (locus_descrim_hasher::equal): Likewise.
>> (next_discriminator_for_locus): Likewise.
>> (assign_discriminator): Add return value.
>> (make_edges): Assign more discriminator if necessary.
>> (make_cond_expr_edges): Likewise.
>> (make_goto_expr_edges): Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.dg/debug/dwarf2/discriminator.c: New Test.
>
> Looks OK to me, but I can't approve patches for tree-cfg.c.
>
> Two comments:
>
> (1) In the C++ conversion, it looks like someone misspelled "discrim"
> in "locus_descrim_hasher".
>
> (2) Is this worth fixing in trunk when we'll eventually switch to a
> per-instruction discriminator?

Yeah, the same problem exists for per-instruction discriminator. This
patch needs to be backported to google-4_7 and 4_8 branches.

Thanks,
Dehao

>
> -cary
Richard Biener May 17, 2013, 8:22 a.m. UTC | #3 
On Wed, May 15, 2013 at 6:50 PM, Cary Coutant <ccoutant@google.com> wrote:
>> gcc/ChangeLog:
>>
>> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
>> (locus_descrim_hasher::equal): Likewise.
>> (next_discriminator_for_locus): Likewise.
>> (assign_discriminator): Add return value.
>> (make_edges): Assign more discriminator if necessary.
>> (make_cond_expr_edges): Likewise.
>> (make_goto_expr_edges): Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.dg/debug/dwarf2/discriminator.c: New Test.
>
> Looks OK to me, but I can't approve patches for tree-cfg.c.
>
> Two comments:
>
> (1) In the C++ conversion, it looks like someone misspelled "discrim"
> in "locus_descrim_hasher".
>
> (2) Is this worth fixing in trunk when we'll eventually switch to a
> per-instruction discriminator?

> This patch fixes a common case where one line is distributed to 3 BBs,
> but only 1 discriminator is assigned.

As far as I can see the patch makes discriminators coarser (by hashing
and comparing on LOCATION_LINE and not on the location).  It also has
the changes like

-  assign_discriminator (entry_locus, then_bb);
+  if (assign_discriminator (entry_locus, then_bb))
+    assign_discriminator (entry_locus, bb);

which is what the comment refers to?  I think those changes are short-sighted
because what happens if the 2nd assign_discriminator call has exactly the
same issue?  Especially with the make_goto_expr_edges change there
may be multiple predecessors where I cannot see how the change is
correct.  Yes, the change changes something and thus may fix the testcase
but it doesn't change things in a predictable way as far as I can see.

So - the change to compare/hash LOCATION_LINE looks good to me,
but the assign_discriminator change needs more explaining.

Richard.

> -cary
Dehao Chen May 17, 2013, 3:48 p.m. UTC | #4 
On Fri, May 17, 2013 at 1:22 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, May 15, 2013 at 6:50 PM, Cary Coutant <ccoutant@google.com> wrote:
>>> gcc/ChangeLog:
>>>
>>> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
>>> (locus_descrim_hasher::equal): Likewise.
>>> (next_discriminator_for_locus): Likewise.
>>> (assign_discriminator): Add return value.
>>> (make_edges): Assign more discriminator if necessary.
>>> (make_cond_expr_edges): Likewise.
>>> (make_goto_expr_edges): Likewise.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * gcc.dg/debug/dwarf2/discriminator.c: New Test.
>>
>> Looks OK to me, but I can't approve patches for tree-cfg.c.
>>
>> Two comments:
>>
>> (1) In the C++ conversion, it looks like someone misspelled "discrim"
>> in "locus_descrim_hasher".
>>
>> (2) Is this worth fixing in trunk when we'll eventually switch to a
>> per-instruction discriminator?
>
>> This patch fixes a common case where one line is distributed to 3 BBs,
>> but only 1 discriminator is assigned.
>
> As far as I can see the patch makes discriminators coarser (by hashing
> and comparing on LOCATION_LINE and not on the location).  It also has
> the changes like
>
> -  assign_discriminator (entry_locus, then_bb);
> +  if (assign_discriminator (entry_locus, then_bb))
> +    assign_discriminator (entry_locus, bb);
>
> which is what the comment refers to?  I think those changes are short-sighted
> because what happens if the 2nd assign_discriminator call has exactly the
> same issue?  Especially with the make_goto_expr_edges change there
> may be multiple predecessors where I cannot see how the change is
> correct.  Yes, the change changes something and thus may fix the testcase
> but it doesn't change things in a predictable way as far as I can see.
>
> So - the change to compare/hash LOCATION_LINE looks good to me,
> but the assign_discriminator change needs more explaining.

The new discriminator assignment algorithm is:

1 FOR_EACH_BB:
2   FOR_EACH_SUCC_EDGE:
3     if (same_line(bb, succ_bb))
4       if (succ_bb has no discriminator)
5          succ_bb->discriminator = new_discriminator
6       else if (bb has no discriminator)
7          bb->discriminator = new_discriminator

Because there are so many places to handle SUCC_EDGE, thus the logic
line #3, #4 is embedded within assign_discriminator function, and the
logic in line #6 is encoded as the return value of
assign_discriminator.

Thanks,
Dehao

>
> Richard.
>
>> -cary
Richard Biener May 21, 2013, 12:34 p.m. UTC | #5 
On Fri, May 17, 2013 at 5:48 PM, Dehao Chen <dehao@google.com> wrote:
> On Fri, May 17, 2013 at 1:22 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, May 15, 2013 at 6:50 PM, Cary Coutant <ccoutant@google.com> wrote:
>>>> gcc/ChangeLog:
>>>>
>>>> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
>>>> (locus_descrim_hasher::equal): Likewise.
>>>> (next_discriminator_for_locus): Likewise.
>>>> (assign_discriminator): Add return value.
>>>> (make_edges): Assign more discriminator if necessary.
>>>> (make_cond_expr_edges): Likewise.
>>>> (make_goto_expr_edges): Likewise.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> * gcc.dg/debug/dwarf2/discriminator.c: New Test.
>>>
>>> Looks OK to me, but I can't approve patches for tree-cfg.c.
>>>
>>> Two comments:
>>>
>>> (1) In the C++ conversion, it looks like someone misspelled "discrim"
>>> in "locus_descrim_hasher".
>>>
>>> (2) Is this worth fixing in trunk when we'll eventually switch to a
>>> per-instruction discriminator?
>>
>>> This patch fixes a common case where one line is distributed to 3 BBs,
>>> but only 1 discriminator is assigned.
>>
>> As far as I can see the patch makes discriminators coarser (by hashing
>> and comparing on LOCATION_LINE and not on the location).  It also has
>> the changes like
>>
>> -  assign_discriminator (entry_locus, then_bb);
>> +  if (assign_discriminator (entry_locus, then_bb))
>> +    assign_discriminator (entry_locus, bb);
>>
>> which is what the comment refers to?  I think those changes are short-sighted
>> because what happens if the 2nd assign_discriminator call has exactly the
>> same issue?  Especially with the make_goto_expr_edges change there
>> may be multiple predecessors where I cannot see how the change is
>> correct.  Yes, the change changes something and thus may fix the testcase
>> but it doesn't change things in a predictable way as far as I can see.
>>
>> So - the change to compare/hash LOCATION_LINE looks good to me,
>> but the assign_discriminator change needs more explaining.
>
> The new discriminator assignment algorithm is:
>
> 1 FOR_EACH_BB:
> 2   FOR_EACH_SUCC_EDGE:
> 3     if (same_line(bb, succ_bb))
> 4       if (succ_bb has no discriminator)
> 5          succ_bb->discriminator = new_discriminator
> 6       else if (bb has no discriminator)
> 7          bb->discriminator = new_discriminator
>
> Because there are so many places to handle SUCC_EDGE, thus the logic
> line #3, #4 is embedded within assign_discriminator function, and the
> logic in line #6 is encoded as the return value of
> assign_discriminator.

Hmm, as of current the code is hardly readable while the above makes sense
(well, apart from what happens if both succ and bb already have a
discriminator).

Can I make you refactor the current code to postpone discriminator assignment
until after make_edges completed, thus, do it in a postprocessing step done
exactly like outlined above?  That way also the whole thing, including
the currently global  discriminator_per_locus can be localized into a single
function.

Thanks,
Richard.


> Thanks,
> Dehao
>
>>
>> Richard.
>>
>>> -cary
Eric Botcazou May 25, 2013, 4:35 p.m. UTC | #6 
> gcc/ChangeLog:
> 
> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
> (locus_descrim_hasher::equal): Likewise.
> (next_discriminator_for_locus): Likewise.
> (assign_discriminator): Add return value.
> (make_edges): Assign more discriminator if necessary.
> (make_cond_expr_edges): Likewise.
> (make_goto_expr_edges): Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> * gcc.dg/debug/dwarf2/discriminator.c: New Test.

The above ChangeLog entries are good, but the ones you installed aren't (wrong 
gcc/ prefix, wrong location for the testsuite entry, several typos, etc).

Please fix.
diff mbox

Patch

Index: gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
===================================================================
--- gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c	(revision 0)
+++ gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c	(revision 0)
@@ -0,0 +1,16 @@ 
+/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-options "-O0 -gdwarf-2" } */
+/* { dg-final { scan-assembler "loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?\n" } } */
+/* { dg-final { scan-assembler "loc \[0-9] 9 \[0-9]( is_stmt \[0-9])? discriminator 2\n" } } */
+/* { dg-final { scan-assembler "loc \[0-9] 9 \[0-9]( is_stmt \[0-9])? discriminator 1\n" } } */
+
+int foo(int n) {
+  int i, ret = 0;
+  for (i = 0; i < n; i++) {
+    if (i % 10 == 1)
+      ret++;
+    else
+      ret--;
+  }
+  return ret;
+}
Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c	(revision 198891)
+++ gcc/tree-cfg.c	(working copy)
@@ -105,7 +105,7 @@  struct locus_descrim_hasher : typed_free_remove <l
 inline hashval_t
 locus_descrim_hasher::hash (const value_type *item)
 {
-  return item->locus;
+  return LOCATION_LINE (item->locus);
 }
 
 /* Equality function for the locus-to-discriminator map.  A and B
@@ -114,7 +114,7 @@  locus_descrim_hasher::hash (const value_type *item
 inline bool
 locus_descrim_hasher::equal (const value_type *a, const compare_type *b)
 {
-  return a->locus == b->locus;
+  return LOCATION_LINE (a->locus) == LOCATION_LINE (b->locus);
 }
 
 static hash_table <locus_descrim_hasher> discriminator_per_locus;
@@ -129,7 +129,7 @@  static void make_cond_expr_edges (basic_block);
 static void make_gimple_switch_edges (basic_block);
 static void make_goto_expr_edges (basic_block);
 static void make_gimple_asm_edges (basic_block);
-static void assign_discriminator (location_t, basic_block);
+static bool assign_discriminator (location_t, basic_block);
 static edge gimple_redirect_edge_and_branch (edge, basic_block);
 static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
 static unsigned int split_critical_edges (void);
@@ -693,7 +693,8 @@  make_edges (void)
         {
 	  make_edge (bb, bb->next_bb, EDGE_FALLTHRU);
 	  if (last)
-            assign_discriminator (gimple_location (last), bb->next_bb);
+            if (assign_discriminator (gimple_location (last), bb->next_bb))
+	      assign_discriminator (gimple_location (last), bb);
 	}
     }
 
@@ -717,7 +718,8 @@  next_discriminator_for_locus (location_t locus)
 
   item.locus = locus;
   item.discriminator = 0;
-  slot = discriminator_per_locus.find_slot_with_hash (&item, locus, INSERT);
+  slot = discriminator_per_locus.find_slot_with_hash (
+      &item, LOCATION_LINE (locus), INSERT);
   gcc_assert (slot);
   if (*slot == HTAB_EMPTY_ENTRY)
     {
@@ -753,21 +755,29 @@  same_line_p (location_t locus1, location_t locus2)
 }
 
 /* Assign a unique discriminator value to block BB if it begins at the same
-   LOCUS as its predecessor block.  */
+   LOCUS as its pred/succ block. Return true if discriminator has already
+   been assigned for BB and needs to assign additional discriminator for
+   BB's predecessor.  */
 
-static void
+static bool
 assign_discriminator (location_t locus, basic_block bb)
 {
   gimple first_in_to_bb, last_in_to_bb;
 
-  if (locus == 0 || bb->discriminator != 0)
-    return;
+  if (locus == 0)
+    return false;
 
   first_in_to_bb = first_non_label_stmt (bb);
   last_in_to_bb = last_stmt (bb);
   if ((first_in_to_bb && same_line_p (locus, gimple_location (first_in_to_bb)))
       || (last_in_to_bb && same_line_p (locus, gimple_location (last_in_to_bb))))
-    bb->discriminator = next_discriminator_for_locus (locus);
+    {
+      if (bb->discriminator != 0)
+	return true;
+      else
+	bb->discriminator = next_discriminator_for_locus (locus);
+    }
+  return false;
 }
 
 /* Create the edges for a GIMPLE_COND starting at block BB.  */
@@ -796,12 +806,14 @@  make_cond_expr_edges (basic_block bb)
   else_stmt = first_stmt (else_bb);
 
   e = make_edge (bb, then_bb, EDGE_TRUE_VALUE);
-  assign_discriminator (entry_locus, then_bb);
+  if (assign_discriminator (entry_locus, then_bb))
+    assign_discriminator (entry_locus, bb);
   e->goto_locus = gimple_location (then_stmt);
   e = make_edge (bb, else_bb, EDGE_FALSE_VALUE);
   if (e)
     {
-      assign_discriminator (entry_locus, else_bb);
+      if (assign_discriminator (entry_locus, else_bb))
+	assign_discriminator (entry_locus, bb);
       e->goto_locus = gimple_location (else_stmt);
     }
 
@@ -1020,7 +1032,8 @@  make_goto_expr_edges (basic_block bb)
       basic_block label_bb = label_to_block (dest);
       edge e = make_edge (bb, label_bb, EDGE_FALLTHRU);
       e->goto_locus = gimple_location (goto_t);
-      assign_discriminator (e->goto_locus, label_bb);
+      if (assign_discriminator (e->goto_locus, label_bb))
+	assign_discriminator (e->goto_locus, bb);
       gsi_remove (&last, true);
       return;
     }