Patchwork Fix incorrect discriminator assignment.

login
register
mail settings
Submitter Dehao Chen
Date May 22, 2013, 2:55 p.m.
Message ID <CAO2gOZV-SDTZ0WKrRTS-9N=ZJmSqm3oPDNzwmTDGxbboLJGE7g@mail.gmail.com>
Download mbox | patch
Permalink /patch/245641/
State New
Headers show

Comments

Dehao Chen - May 22, 2013, 2:55 p.m.
Sounds reasonable. The patch is updated, bootstrapped and passed all
regression test.

Dehao

On Tue, May 21, 2013 at 5:34 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> 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
Cary Coutant - May 22, 2013, 4:40 p.m.
@@ -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);
 }


While you're in that part of the code, could you fix the spelling of
locus_descrim_hasher? Should be "locus_discrim_hasher".

-cary
Dehao Chen - May 22, 2013, 4:46 p.m.
Sure, will update the patch for that.

Dehao

On Wed, May 22, 2013 at 9:40 AM, Cary Coutant <ccoutant@google.com> wrote:
> @@ -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);
>  }
>
>
> While you're in that part of the code, could you fix the spelling of
> locus_descrim_hasher? Should be "locus_discrim_hasher".
>
> -cary

Patch

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;
@@ -125,11 +125,11 @@  static void factor_computed_gotos (void);
 
 /* Edges.  */
 static void make_edges (void);
+static void assign_discriminators (void);
 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 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);
@@ -231,6 +231,7 @@  build_gimple_cfg (gimple_seq seq)
   /* Create the edges of the flowgraph.  */
   discriminator_per_locus.create (13);
   make_edges ();
+  assign_discriminators ();
   cleanup_dead_labels ();
   discriminator_per_locus.dispose ();
 }
@@ -690,11 +691,7 @@  make_edges (void)
 	fallthru = true;
 
       if (fallthru)
-        {
-	  make_edge (bb, bb->next_bb, EDGE_FALLTHRU);
-	  if (last)
-            assign_discriminator (gimple_location (last), bb->next_bb);
-	}
+	make_edge (bb, bb->next_bb, EDGE_FALLTHRU);
     }
 
   if (root_omp_region)
@@ -717,7 +714,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)
     {
@@ -752,22 +750,37 @@  same_line_p (location_t locus1, location_t locus2)
           && filename_cmp (from.file, to.file) == 0);
 }
 
-/* Assign a unique discriminator value to block BB if it begins at the same
-   LOCUS as its predecessor block.  */
+/* Assign discriminators to each basic block.  */
 
 static void
-assign_discriminator (location_t locus, basic_block bb)
+assign_discriminators (void)
 {
-  gimple first_in_to_bb, last_in_to_bb;
+  basic_block bb;
 
-  if (locus == 0 || bb->discriminator != 0)
-    return;
+  FOR_EACH_BB (bb)
+    {
+      edge e;
+      edge_iterator ei;
+      gimple last = last_stmt (bb);
+      location_t locus = last ? gimple_location (last) : UNKNOWN_LOCATION;
 
-  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 (locus == UNKNOWN_LOCATION)
+	continue;
+
+      FOR_EACH_EDGE (e, ei, bb->succs)
+	{
+	  gimple first = first_non_label_stmt (e->dest);
+	  gimple last = last_stmt (e->dest);
+	  if ((first && same_line_p (locus, gimple_location (first)))
+	      || (last && same_line_p (locus, gimple_location (last))))
+	    {
+	      if (e->dest->discriminator != 0 && bb->discriminator == 0)
+		bb->discriminator = next_discriminator_for_locus (locus);
+	      else
+		e->dest->discriminator = next_discriminator_for_locus (locus);
+	    }
+	}
+    }
 }
 
 /* Create the edges for a GIMPLE_COND starting at block BB.  */
@@ -780,13 +793,10 @@  make_cond_expr_edges (basic_block bb)
   basic_block then_bb, else_bb;
   tree then_label, else_label;
   edge e;
-  location_t entry_locus;
 
   gcc_assert (entry);
   gcc_assert (gimple_code (entry) == GIMPLE_COND);
 
-  entry_locus = gimple_location (entry);
-
   /* Entry basic blocks for each component.  */
   then_label = gimple_cond_true_label (entry);
   else_label = gimple_cond_false_label (entry);
@@ -796,14 +806,10 @@  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);
   e->goto_locus = gimple_location (then_stmt);
   e = make_edge (bb, else_bb, EDGE_FALSE_VALUE);
   if (e)
-    {
-      assign_discriminator (entry_locus, else_bb);
-      e->goto_locus = gimple_location (else_stmt);
-    }
+    e->goto_locus = gimple_location (else_stmt);
 
   /* We do not need the labels anymore.  */
   gimple_cond_set_true_label (entry, NULL_TREE);
@@ -923,11 +929,8 @@  static void
 make_gimple_switch_edges (basic_block bb)
 {
   gimple entry = last_stmt (bb);
-  location_t entry_locus;
   size_t i, n;
 
-  entry_locus = gimple_location (entry);
-
   n = gimple_switch_num_labels (entry);
 
   for (i = 0; i < n; ++i)
@@ -935,7 +938,6 @@  make_gimple_switch_edges (basic_block bb)
       tree lab = CASE_LABEL (gimple_switch_label (entry, i));
       basic_block label_bb = label_to_block (lab);
       make_edge (bb, label_bb, 0);
-      assign_discriminator (entry_locus, label_bb);
     }
 }
 
@@ -1020,7 +1022,6 @@  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);
       gsi_remove (&last, true);
       return;
     }
@@ -1035,7 +1036,6 @@  static void
 make_gimple_asm_edges (basic_block bb)
 {
   gimple stmt = last_stmt (bb);
-  location_t stmt_loc = gimple_location (stmt);
   int i, n = gimple_asm_nlabels (stmt);
 
   for (i = 0; i < n; ++i)
@@ -1043,7 +1043,6 @@  make_gimple_asm_edges (basic_block bb)
       tree label = TREE_VALUE (gimple_asm_label_op (stmt, i));
       basic_block label_bb = label_to_block (label);
       make_edge (bb, label_bb, 0);
-      assign_discriminator (stmt_loc, label_bb);
     }
 }
 
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;
+}