[PATCH] Update source location for PRE inserted stmt

[PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Hi,

This patch aims to improve debugging of optimized code. It ensures
that PRE inserted statements have the same source location as the
statement at the insertion point, instead of UNKNOWN_LOCATION.

Bootstrapped on x86_64, and passed gcc regression tests and gdb
regression tests.

Is it okay for trunk?

Thanks,
Dehao

gcc/ChangeLog:
2012-10-30  Dehao Chen  <dehao@google.com>

        * tree-ssa-pre.c (insert_into_pred_update_location): New Function.
        (insert_into_preds_of_block): Update source location for inserted stmts.

gcc/testsuite/ChangeLog:
2012-10-30  Dehao Chen  <dehao@google.com>

        * gcc.dg/debug/dwarf2/pre.c: New testcase.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 5608 bytes --]

Index: testsuite/g++.dg/debug/dwarf2/block.C
===================================================================
--- testsuite/g++.dg/debug/dwarf2/block.C	(revision 0)
+++ testsuite/g++.dg/debug/dwarf2/block.C	(revision 0)
@@ -0,0 +1,29 @@
+// Compiler should not generate too many lexical blocks for this function.
+// { dg-do compile { target { i?86-*-* x86_64-*-* } } }
+// { dg-options "-O0 -fno-exceptions -g -dA" }
+
+union UElement {
+    void* pointer;
+    int integer;
+};
+struct UColToken {
+  unsigned source;
+  unsigned char **rulesToParseHdl;
+};
+
+int uhash_hashTokens(const union UElement k)
+{
+  int hash = 0;
+  struct UColToken *key = (struct UColToken *)k.pointer;
+  if (key != 0) {
+    int len = (key->source & 0xFF000000)>>24;
+    int inc = ((len - 32) / 32) + 1;
+    const unsigned char *p = (key->source & 0x00FFFFFF)
+			     + *(key->rulesToParseHdl);
+    const unsigned char *limit = p + len;
+    hash = *p + *limit;
+  }
+  return hash;
+}
+
+// { dg-final { scan-assembler-not "LBB10" } }
Index: tree-eh.c
===================================================================
--- tree-eh.c	(revision 192809)
+++ tree-eh.c	(working copy)
@@ -739,6 +739,7 @@ do_return_redirection (struct goto_queue_node *q,
     gimple_seq_add_seq (&q->repl_stmt, mod);
 
   x = gimple_build_goto (finlab);
+  gimple_set_location (x, q->location);
   gimple_seq_add_stmt (&q->repl_stmt, x);
 }
 
@@ -758,6 +759,7 @@ do_goto_redirection (struct goto_queue_node *q, tr
     gimple_seq_add_seq (&q->repl_stmt, mod);
 
   x = gimple_build_goto (finlab);
+  gimple_set_location (x, q->location);
   gimple_seq_add_stmt (&q->repl_stmt, x);
 }
 
@@ -857,6 +859,7 @@ frob_into_branch_around (gimple tp, eh_region regi
       if (!over)
 	over = create_artificial_label (loc);
       x = gimple_build_goto (over);
+      gimple_set_location (x, loc);
       gimple_seq_add_stmt (&cleanup, x);
     }
   gimple_seq_add_seq (&eh_seq, cleanup);
@@ -1085,6 +1088,7 @@ lower_try_finally_nofallthru (struct leh_state *st
 	  emit_post_landing_pad (&eh_seq, tf->region);
 
 	  x = gimple_build_goto (lab);
+	  gimple_set_location (x, gimple_location (tf->try_finally_expr));
 	  gimple_seq_add_stmt (&eh_seq, x);
 	}
     }
@@ -1223,6 +1227,7 @@ lower_try_finally_copy (struct leh_state *state, s
 
       tmp = lower_try_finally_fallthru_label (tf);
       x = gimple_build_goto (tmp);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&new_stmt, x);
     }
 
@@ -1395,6 +1400,7 @@ lower_try_finally_switch (struct leh_state *state,
 
       tmp = lower_try_finally_fallthru_label (tf);
       x = gimple_build_goto (tmp);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&switch_body, x);
     }
 
@@ -1423,6 +1429,7 @@ lower_try_finally_switch (struct leh_state *state,
       gimple_seq_add_stmt (&eh_seq, x);
 
       x = gimple_build_goto (finally_label);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&eh_seq, x);
 
       tmp = build_int_cst (integer_type_node, eh_index);
Index: expr.c
===================================================================
--- expr.c	(revision 192809)
+++ expr.c	(working copy)
@@ -5030,7 +5030,7 @@ store_expr (tree exp, rtx target, int call_param_p
 {
   rtx temp;
   rtx alt_rtl = NULL_RTX;
-  location_t loc = EXPR_LOCATION (exp);
+  location_t loc = curr_insn_location ();
 
   if (VOID_TYPE_P (TREE_TYPE (exp)))
     {
@@ -7869,31 +7869,7 @@ expand_expr_real (tree exp, rtx target, enum machi
       return ret ? ret : const0_rtx;
     }
 
-  /* If this is an expression of some kind and it has an associated line
-     number, then emit the line number before expanding the expression.
-
-     We need to save and restore the file and line information so that
-     errors discovered during expansion are emitted with the right
-     information.  It would be better of the diagnostic routines
-     used the file/line information embedded in the tree nodes rather
-     than globals.  */
-  if (cfun && EXPR_HAS_LOCATION (exp))
-    {
-      location_t saved_location = input_location;
-      location_t saved_curr_loc = curr_insn_location ();
-      input_location = EXPR_LOCATION (exp);
-      set_curr_insn_location (input_location);
-
-      ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl);
-
-      input_location = saved_location;
-      set_curr_insn_location (saved_curr_loc);
-    }
-  else
-    {
-      ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl);
-    }
-
+  ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl);
   return ret;
 }
 
@@ -9250,8 +9226,15 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	g = SSA_NAME_DEF_STMT (exp);
       if (g)
 	{
-	  rtx r = expand_expr_real (gimple_assign_rhs_to_tree (g), target,
-				    tmode, modifier, NULL);
+	  rtx r;
+	  location_t saved_loc = input_location;
+
+	  input_location = gimple_location (g);
+	  set_curr_insn_location (input_location);
+	  r = expand_expr_real (gimple_assign_rhs_to_tree (g), target,
+				tmode, modifier, NULL);
+	  input_location = saved_loc;
+	  set_curr_insn_location (saved_loc);
 	  if (REG_P (r) && !REG_EXPR (r))
 	    set_reg_attrs_for_decl_rtl (SSA_NAME_VAR (exp), r);
 	  return r;
@@ -9481,7 +9464,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	       with non-BLKmode values.  */
 	    gcc_assert (GET_MODE (ret) != BLKmode);
 
-	    val = build_decl (EXPR_LOCATION (exp),
+	    val = build_decl (curr_insn_location (),
 			      VAR_DECL, NULL, TREE_TYPE (exp));
 	    DECL_ARTIFICIAL (val) = 1;
 	    DECL_IGNORED_P (val) = 1;

Re: [PATCH] Update source location for PRE inserted stmt

[Steven Bosscher]

On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote:
> This patch aims to improve debugging of optimized code. It ensures
> that PRE inserted statements have the same source location as the
> statement at the insertion point, instead of UNKNOWN_LOCATION.

Wrong patch attached.

However, is it really better to have the location of the insertion
point than to have UNKNOWN_LOCATION? It's not where the value is
computed in the source program...

Ciao!
Steven

Re: [PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Sorry, new patch attached...

On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote:
>> This patch aims to improve debugging of optimized code. It ensures
>> that PRE inserted statements have the same source location as the
>> statement at the insertion point, instead of UNKNOWN_LOCATION.
>
> Wrong patch attached.
>
> However, is it really better to have the location of the insertion
> point than to have UNKNOWN_LOCATION? It's not where the value is
> computed in the source program...

Setting it to UNKNOWN_LOCATION is expecting it to inherit source
location from its previous stmt. However, backend optimization could
optimize the previous stmt away, making the inserted stmt with random
location. This patch just enforce the location to be the same as
previous stmt while insertion, so that random stuff does not happen.

In general, we want to reduce the number of UNKNOWN_LOCATIONS emitted.
At least we do not want to see UNKNOWN_LOCATION at the beginning of
any BB.

Thanks,
Dehao
>
> Ciao!
> Steven

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2769 bytes --]

Index: gcc/tree-ssa-pre.c
===================================================================
--- gcc/tree-ssa-pre.c	(revision 192809)
+++ gcc/tree-ssa-pre.c	(working copy)
@@ -3039,6 +3039,20 @@ inhibit_phi_insertion (basic_block bb, pre_expr ex
   return false;
 }
 
+static void
+insert_into_pred_update_location (edge pred, gimple_seq stmts)
+{
+  gimple_stmt_iterator gsi;
+  gimple stmt = last_stmt (pred->src);
+  location_t location = stmt ? gimple_location (stmt)
+			     : UNKNOWN_LOCATION;
+  if (location != UNKNOWN_LOCATION)
+    for (gsi = gsi_start (stmts); !gsi_end_p (gsi); gsi_next (&gsi))
+      if (gimple_location (gsi_stmt (gsi)) == UNKNOWN_LOCATION)
+	gimple_set_location (gsi_stmt (gsi), location);
+  gsi_insert_seq_on_edge (pred, stmts);
+}
+
 /* Insert the to-be-made-available values of expression EXPRNUM for each
    predecessor, stored in AVAIL, into the predecessors of BLOCK, and
    merge the result with a phi node, given the same value number as
@@ -3094,7 +3108,7 @@ insert_into_preds_of_block (basic_block block, uns
 	  builtexpr = create_expression_by_pieces (bprime, eprime,
 						   &stmts, type);
 	  gcc_assert (!(pred->flags & EDGE_ABNORMAL));
-	  gsi_insert_seq_on_edge (pred, stmts);
+	  insert_into_pred_update_location (pred, stmts);
 	  VEC_replace (pre_expr, avail, pred->dest_idx,
 		       get_or_alloc_expr_for_name (builtexpr));
 	  insertions = true;
@@ -3132,7 +3146,7 @@ insert_into_preds_of_block (basic_block block, uns
 						SSA_NAME_VERSION (lhs));
 			      gimple_set_plf (stmt, NECESSARY, false);
 			    }
-			  gsi_insert_seq_on_edge (pred, stmts);
+			  insert_into_pred_update_location (pred, stmts);
 			}
 		      VEC_replace (pre_expr, avail, pred->dest_idx,
 				   get_or_alloc_expr_for_name (forcedexpr));
@@ -3177,7 +3191,7 @@ insert_into_preds_of_block (basic_block block, uns
 			bitmap_set_bit (inserted_exprs, SSA_NAME_VERSION (lhs));
 		      gimple_set_plf (stmt, NECESSARY, false);
 		    }
-		  gsi_insert_seq_on_edge (pred, stmts);
+		  insert_into_pred_update_location (pred, stmts);
 		}
 	      VEC_replace (pre_expr, avail, pred->dest_idx,
 			   get_or_alloc_expr_for_name (forcedexpr));
Index: gcc/testsuite/gcc.dg/debug/dwarf2/pre.c
===================================================================
--- gcc/testsuite/gcc.dg/debug/dwarf2/pre.c	(revision 0)
+++ gcc/testsuite/gcc.dg/debug/dwarf2/pre.c	(revision 0)
@@ -0,0 +1,18 @@
+// This test makes sure PRE will not optimize the debug info away.
+// { dg-do compile { target { i?86-*-* x86_64-*-* } } }
+// { dg-options "-O2 -g -dA" }
+extern int x;
+
+int abc (int *a)
+{
+  int ret = 0;
+
+  if (x > 0)
+    ret += *a;
+  else
+    a++;
+
+  ret += *a;
+  return ret;
+}
+// { dg-final { scan-assembler "pre.c:13" } }

Re: [PATCH] Update source location for PRE inserted stmt

[Xinliang David Li]

It will make the location info for the newly synthesized stmt more
deterministic, I think.

David

On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote:
>> This patch aims to improve debugging of optimized code. It ensures
>> that PRE inserted statements have the same source location as the
>> statement at the insertion point, instead of UNKNOWN_LOCATION.
>
> Wrong patch attached.
>
> However, is it really better to have the location of the insertion
> point than to have UNKNOWN_LOCATION? It's not where the value is
> computed in the source program...
>
> Ciao!
> Steven

Re: [PATCH] Update source location for PRE inserted stmt

[Richard Biener]

On Wed, Oct 31, 2012 at 12:57 AM, Xinliang David Li <davidxl@google.com> wrote:
> It will make the location info for the newly synthesized stmt more
> deterministic, I think.

Maybe, but it will increase the jumpiness in the debugger without actually
being accurate, no?  For example if the partially redundant expression is

  i + j;

then when computed at the insertion point the values of i and j do not
necessarily reflect the computed value!  Instead we may compute the
result of i + j using completely different components / operation.

Thus I think inserted expressions should not have any debug information
at all because they do not correspond to a source line.

Richard.

> David
>
> On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>> On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote:
>>> This patch aims to improve debugging of optimized code. It ensures
>>> that PRE inserted statements have the same source location as the
>>> statement at the insertion point, instead of UNKNOWN_LOCATION.
>>
>> Wrong patch attached.
>>
>> However, is it really better to have the location of the insertion
>> point than to have UNKNOWN_LOCATION? It's not where the value is
>> computed in the source program...
>>
>> Ciao!
>> Steven

Re: [PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

On Wed, Oct 31, 2012 at 2:34 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Oct 31, 2012 at 12:57 AM, Xinliang David Li <davidxl@google.com> wrote:
>> It will make the location info for the newly synthesized stmt more
>> deterministic, I think.
>
> Maybe, but it will increase the jumpiness in the debugger without actually
> being accurate, no?  For example if the partially redundant expression is

Sometimes, it is necessary to have a jump in gdb, especially when
control flow changes. For the test case I gave, people may want to
know if the branch in line 10 is taken or not. Without this patch, the
gdb simply jump from line 10 to line 15 if the branch is not taken.
But with the patch, people get the correct control flow.

One more thing, for AutoFDO to work, we want to make sure debug info
is correct at control flow boundaries. That is why we would prefer
deterministic location info, instead of randomly inherit debug info
from previous BB.

Thanks,
Dehao

>
>   i + j;
>
> then when computed at the insertion point the values of i and j do not
> necessarily reflect the computed value!  Instead we may compute the
> result of i + j using completely different components / operation.
>
> Thus I think inserted expressions should not have any debug information
> at all because they do not correspond to a source line.
>
> Richard.
>
>> David
>>
>> On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>>> On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote:
>>>> This patch aims to improve debugging of optimized code. It ensures
>>>> that PRE inserted statements have the same source location as the
>>>> statement at the insertion point, instead of UNKNOWN_LOCATION.
>>>
>>> Wrong patch attached.
>>>
>>> However, is it really better to have the location of the insertion
>>> point than to have UNKNOWN_LOCATION? It's not where the value is
>>> computed in the source program...
>>>
>>> Ciao!
>>> Steven

Re: [PATCH] Update source location for PRE inserted stmt

[Xinliang David Li]

Dehao's patch will make the debugging of the following code (-g -O2)
less jumpy.   After the testing of x > 0, it should go to line 'a++'.
 Without the fix, when stepping through 'abc', the lines covered are
6, 4, 11, 13. With the fix it should be 6, 9, 11, 13 -- much better.

David




1. int x;

2. __attribute__((noinline)) int abc (int *a)
3. {
4.  int ret = 0;
5.
6.  if (x > 0)
7.    ret += *a;
8.  else
9.    a++;
10.
11.  ret += *a;
12.  return ret;
13 }


int main()
{
  int a = 0;

   x = -1;
   return abc ( &a);

}

On Wed, Oct 31, 2012 at 2:34 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Oct 31, 2012 at 12:57 AM, Xinliang David Li <davidxl@google.com> wrote:
>> It will make the location info for the newly synthesized stmt more
>> deterministic, I think.
>
> Maybe, but it will increase the jumpiness in the debugger without actually
> being accurate, no?  For example if the partially redundant expression is
>
>   i + j;
>
> then when computed at the insertion point the values of i and j do not
> necessarily reflect the computed value!  Instead we may compute the
> result of i + j using completely different components / operation.
>
> Thus I think inserted expressions should not have any debug information
> at all because they do not correspond to a source line.
>
> Richard.
>
>> David
>>
>> On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>>> On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote:
>>>> This patch aims to improve debugging of optimized code. It ensures
>>>> that PRE inserted statements have the same source location as the
>>>> statement at the insertion point, instead of UNKNOWN_LOCATION.
>>>
>>> Wrong patch attached.
>>>
>>> However, is it really better to have the location of the insertion
>>> point than to have UNKNOWN_LOCATION? It's not where the value is
>>> computed in the source program...
>>>
>>> Ciao!
>>> Steven

Re: [PATCH] Update source location for PRE inserted stmt

[Richard Biener]

On Wed, Oct 31, 2012 at 8:02 PM, Xinliang David Li <davidxl@google.com> wrote:
> Dehao's patch will make the debugging of the following code (-g -O2)
> less jumpy.   After the testing of x > 0, it should go to line 'a++'.
>  Without the fix, when stepping through 'abc', the lines covered are
> 6, 4, 11, 13. With the fix it should be 6, 9, 11, 13 -- much better.

I am not convinced.  Btw, you do not comment my example at all.
For less jumpiness no line number for inserted stmts works as well.

Richard.

> David
>
>
>
>
> 1. int x;
>
> 2. __attribute__((noinline)) int abc (int *a)
> 3. {
> 4.  int ret = 0;
> 5.
> 6.  if (x > 0)
> 7.    ret += *a;
> 8.  else
> 9.    a++;
> 10.
> 11.  ret += *a;
> 12.  return ret;
> 13 }
>
>
> int main()
> {
>   int a = 0;
>
>    x = -1;
>    return abc ( &a);
>
> }
>
> On Wed, Oct 31, 2012 at 2:34 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Oct 31, 2012 at 12:57 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> It will make the location info for the newly synthesized stmt more
>>> deterministic, I think.
>>
>> Maybe, but it will increase the jumpiness in the debugger without actually
>> being accurate, no?  For example if the partially redundant expression is
>>
>>   i + j;
>>
>> then when computed at the insertion point the values of i and j do not
>> necessarily reflect the computed value!  Instead we may compute the
>> result of i + j using completely different components / operation.
>>
>> Thus I think inserted expressions should not have any debug information
>> at all because they do not correspond to a source line.
>>
>> Richard.
>>
>>> David
>>>
>>> On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>>>> On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote:
>>>>> This patch aims to improve debugging of optimized code. It ensures
>>>>> that PRE inserted statements have the same source location as the
>>>>> statement at the insertion point, instead of UNKNOWN_LOCATION.
>>>>
>>>> Wrong patch attached.
>>>>
>>>> However, is it really better to have the location of the insertion
>>>> point than to have UNKNOWN_LOCATION? It's not where the value is
>>>> computed in the source program...
>>>>
>>>> Ciao!
>>>> Steven

Re: [PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

2. __attribute__((noinline)) int abc (int *a)
3. {
4.  int ret = 0;
5.
6.  if (x > 0)
7.    ret += *a;
8.  else
9.    a++;
10.
11.  ret += *a;
12.  return ret;
13 }

In terms of jumpiness, without the patch, the jump sequence is:
2 -> 13 -> 4 -> 11 -> 13

With the patch, the jump sequence is:
2-> 9 -> 4 -> 11 -> 13

I think the patch does not increase the jumpiness, while it
significantly improves coverage.

Thanks,
Dehao

Re: [PATCH] Update source location for PRE inserted stmt

[Xinliang David Li]

On Sun, Nov 4, 2012 at 8:07 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Oct 31, 2012 at 8:02 PM, Xinliang David Li <davidxl@google.com> wrote:
>> Dehao's patch will make the debugging of the following code (-g -O2)
>> less jumpy.   After the testing of x > 0, it should go to line 'a++'.
>>  Without the fix, when stepping through 'abc', the lines covered are
>> 6, 4, 11, 13. With the fix it should be 6, 9, 11, 13 -- much better.
>
> I am not convinced.  Btw, you do not comment my example at all.

Because I was not sure what the example was trying to illustrate.
Let's make it more concrete:

Original program:

if (...)
{
    .. = i + j;
}
else
{
   // (1)
   ..
}

... = i + j;  // (2)

After PRE:

if (...)
{
   t = i + j;
   .. = t;
}
else
{
    // insertion point at 1
    t = i + j;   // New statement
   ..
}

.. = t; // (2)


What is the 'different operation/component' you are referring to in the example?


> For less jumpiness no line number for inserted stmts works as well.


Those compiler generated statements do not have source origins, but
they need to have debug location information attached so that
information collected for them can be correlated with program
constructs (such as CFG). One of the natural way is to use the source
location associated with the program point where they are inserted.

David

>
> Richard.
>
>> David
>>
>>
>>
>>
>> 1. int x;
>>
>> 2. __attribute__((noinline)) int abc (int *a)
>> 3. {
>> 4.  int ret = 0;
>> 5.
>> 6.  if (x > 0)
>> 7.    ret += *a;
>> 8.  else
>> 9.    a++;
>> 10.
>> 11.  ret += *a;
>> 12.  return ret;
>> 13 }
>>
>>
>> int main()
>> {
>>   int a = 0;
>>
>>    x = -1;
>>    return abc ( &a);
>>
>> }
>>
>> On Wed, Oct 31, 2012 at 2:34 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Oct 31, 2012 at 12:57 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> It will make the location info for the newly synthesized stmt more
>>>> deterministic, I think.
>>>
>>> Maybe, but it will increase the jumpiness in the debugger without actually
>>> being accurate, no?  For example if the partially redundant expression is
>>>
>>>   i + j;
>>>
>>> then when computed at the insertion point the values of i and j do not
>>> necessarily reflect the computed value!  Instead we may compute the
>>> result of i + j using completely different components / operation.
>>>
>>> Thus I think inserted expressions should not have any debug information
>>> at all because they do not correspond to a source line.
>>>
>>> Richard.
>>>
>>>> David
>>>>
>>>> On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>>>>> On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote:
>>>>>> This patch aims to improve debugging of optimized code. It ensures
>>>>>> that PRE inserted statements have the same source location as the
>>>>>> statement at the insertion point, instead of UNKNOWN_LOCATION.
>>>>>
>>>>> Wrong patch attached.
>>>>>
>>>>> However, is it really better to have the location of the insertion
>>>>> point than to have UNKNOWN_LOCATION? It's not where the value is
>>>>> computed in the source program...
>>>>>
>>>>> Ciao!
>>>>> Steven

Re: [PATCH] Update source location for PRE inserted stmt

[Eric Botcazou]

> Those compiler generated statements do not have source origins, but
> they need to have debug location information attached so that
> information collected for them can be correlated with program
> constructs (such as CFG). One of the natural way is to use the source
> location associated with the program point where they are inserted.

No, there is nothing natural (and this can even be wrong).  The statements 
must have the source location corresponding to the source construct they are 
generated for, which may be totally different from that of the insertion 
point.  Yes, that can generate jumpiness in GDB, but this is far better that 
breaking the coverage info by giving the same source location to instructions 
that have different coverage status.

-- 
Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

> No, there is nothing natural (and this can even be wrong).  The statements
> must have the source location corresponding to the source construct they are
> generated for, which may be totally different from that of the insertion
> point.  Yes, that can generate jumpiness in GDB, but this is far better that
> breaking the coverage info by giving the same source location to instructions
> that have different coverage status.

For the unittest I provided, setting the inserted stmt with
UNKNOWN_LOCATION will:

* break the coverage
* increase jumpiness in gdb

Setting location to UNKNOWN_LOCATION is like setting it to random
because compiler may put this stmt as the entry point of a BB (as in
the unittest). Thus setting deterministic locations for inserted stmt
will improve debugability and reduce jumpiness.

Thanks,
Dehao

>
> --
> Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Xinliang David Li]

For the example I listed, the new statement is generated for source
construct at program point (2). However unlike simple code motion, (2)
is not going away after PRE. How would setting the location of the new
statement at the insertion point break coverage? Besides, the new
statement won't create 'false' coverage for the insertion point either
as the there are existing statements at that point where the new stmt
inherits the location from.

David

On Mon, Nov 5, 2012 at 3:54 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Those compiler generated statements do not have source origins, but
>> they need to have debug location information attached so that
>> information collected for them can be correlated with program
>> constructs (such as CFG). One of the natural way is to use the source
>> location associated with the program point where they are inserted.
>
> No, there is nothing natural (and this can even be wrong).  The statements
> must have the source location corresponding to the source construct they are
> generated for, which may be totally different from that of the insertion
> point.  Yes, that can generate jumpiness in GDB, but this is far better that
> breaking the coverage info by giving the same source location to instructions
> that have different coverage status.
>
> --
> Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Diego Novillo]

On 2012-11-05 06:54 , Eric Botcazou wrote:
>> Those compiler generated statements do not have source origins, but
>> they need to have debug location information attached so that
>> information collected for them can be correlated with program
>> constructs (such as CFG). One of the natural way is to use the source
>> location associated with the program point where they are inserted.
>
> No, there is nothing natural (and this can even be wrong).  The statements
> must have the source location corresponding to the source construct they are
> generated for, which may be totally different from that of the insertion
> point.  Yes, that can generate jumpiness in GDB, but this is far better that
> breaking the coverage info by giving the same source location to instructions
> that have different coverage status.

But UNKNOWN_LOCATION is effectively wrong as well.  If other 
optimizations move the statements above the inserted instruction, then 
the new instruction ends up inheriting whatever location happens to be 
in the previous statement.

Fixing the location when the statement is inserted will reduce this 
randomness.  I'm not sure I understand why you think this will break 
coverage.  The examples given in this thread were helped by this 
location fixing.

Do you have any counterexample?  I may be missing something here.


Thanks.  Diego.

Re: [PATCH] Update source location for PRE inserted stmt

[Eric Botcazou]

> But UNKNOWN_LOCATION is effectively wrong as well.  If other
> optimizations move the statements above the inserted instruction, then
> the new instruction ends up inheriting whatever location happens to be
> in the previous statement.

That's correct, UNKNOWN_LOCATION isn't a panacea either, although it can help 
in some cases.  The only safe thing to do is to put the exact source location 
of the statement, i.e. the location of the source construct for which the 
statement has been generated.

> Fixing the location when the statement is inserted will reduce this
> randomness.  I'm not sure I understand why you think this will break
> coverage.  The examples given in this thread were helped by this
> location fixing.

What do you call randomness exactly?  GDB jumpiness?  I'm a little wary of 
fixing GDB jumpiness by distorting the debug info.  Ian has clearly outlined 
the correct approach to addressing it.

Note that I didn't specifically reply to Dehao's patch here, which might be 
acceptable in the end, only to David's seemingly general statement about the 
"natural way" of putting a location on these statements.

-- 
Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

On Thu, Nov 15, 2012 at 8:46 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> > But UNKNOWN_LOCATION is effectively wrong as well.  If other
> > optimizations move the statements above the inserted instruction, then
> > the new instruction ends up inheriting whatever location happens to be
> > in the previous statement.
>
> That's correct, UNKNOWN_LOCATION isn't a panacea either, although it can help
> in some cases.  The only safe thing to do is to put the exact source location
> of the statement, i.e. the location of the source construct for which the
> statement has been generated.
>
> > Fixing the location when the statement is inserted will reduce this
> > randomness.  I'm not sure I understand why you think this will break
> > coverage.  The examples given in this thread were helped by this
> > location fixing.
>
> What do you call randomness exactly?  GDB jumpiness?  I'm a little wary of
> fixing GDB jumpiness by distorting the debug info.  Ian has clearly outlined
> the correct approach to addressing it.

The randomness here means that if we set UNKNOWN_LOCATION to insn, it
can get source location anywhere. Even with some small code layout
changes, the location for that insn could change. I would hope that in
the future, we add an assertion when emitting instruction to enforce
that INSN_LOCATION is never UNKNOWN_LOCATION. So when generate new
instructions/stmts, if we can surely know where it is coming from, it
is fine. Otherwise, instead of using UNKNOWN_LOCATION, we will set its
location to where it is inserted. How does that sound?

Thanks,
Dehao

>
>
> Note that I didn't specifically reply to Dehao's patch here, which might be
> acceptable in the end, only to David's seemingly general statement about the
> "natural way" of putting a location on these statements.
>
> --
> Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Eric Botcazou]

> The randomness here means that if we set UNKNOWN_LOCATION to insn, it
> can get source location anywhere. Even with some small code layout
> changes, the location for that insn could change. I would hope that in
> the future, we add an assertion when emitting instruction to enforce
> that INSN_LOCATION is never UNKNOWN_LOCATION. So when generate new
> instructions/stmts, if we can surely know where it is coming from, it
> is fine. Otherwise, instead of using UNKNOWN_LOCATION, we will set its
> location to where it is inserted. How does that sound?

Still the same problem: you cannot make that a general rule, since you don't 
know the coverage status of the instruction just above the insertion point.
If a later optimization moves the new statements around, you may well end up 
with wrong coverage info.

-- 
Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Xinliang David Li]

I probably made too general statement in this topic. However for the
PRE case, I believe the choice of not using UNKNOWN location is still
better.

thanks,

David

On Thu, Nov 15, 2012 at 9:23 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> The randomness here means that if we set UNKNOWN_LOCATION to insn, it
>> can get source location anywhere. Even with some small code layout
>> changes, the location for that insn could change. I would hope that in
>> the future, we add an assertion when emitting instruction to enforce
>> that INSN_LOCATION is never UNKNOWN_LOCATION. So when generate new
>> instructions/stmts, if we can surely know where it is coming from, it
>> is fine. Otherwise, instead of using UNKNOWN_LOCATION, we will set its
>> location to where it is inserted. How does that sound?
>
> Still the same problem: you cannot make that a general rule, since you don't
> know the coverage status of the instruction just above the insertion point.
> If a later optimization moves the new statements around, you may well end up
> with wrong coverage info.
>
> --
> Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

Yeah, at least for the unittest I provided, the coverage info will be
wrong without the patch.

Thanks,
Dehao

On Thu, Nov 15, 2012 at 10:30 AM, Xinliang David Li <davidxl@google.com> wrote:
> I probably made too general statement in this topic. However for the
> PRE case, I believe the choice of not using UNKNOWN location is still
> better.
>
> thanks,
>
> David
>
> On Thu, Nov 15, 2012 at 9:23 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> The randomness here means that if we set UNKNOWN_LOCATION to insn, it
>>> can get source location anywhere. Even with some small code layout
>>> changes, the location for that insn could change. I would hope that in
>>> the future, we add an assertion when emitting instruction to enforce
>>> that INSN_LOCATION is never UNKNOWN_LOCATION. So when generate new
>>> instructions/stmts, if we can surely know where it is coming from, it
>>> is fine. Otherwise, instead of using UNKNOWN_LOCATION, we will set its
>>> location to where it is inserted. How does that sound?
>>
>> Still the same problem: you cannot make that a general rule, since you don't
>> know the coverage status of the instruction just above the insertion point.
>> If a later optimization moves the new statements around, you may well end up
>> with wrong coverage info.
>>
>> --
>> Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Richard Biener]

On Thu, Nov 15, 2012 at 5:46 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> But UNKNOWN_LOCATION is effectively wrong as well.  If other
>> optimizations move the statements above the inserted instruction, then
>> the new instruction ends up inheriting whatever location happens to be
>> in the previous statement.
>
> That's correct, UNKNOWN_LOCATION isn't a panacea either, although it can help
> in some cases.  The only safe thing to do is to put the exact source location
> of the statement, i.e. the location of the source construct for which the
> statement has been generated.

There may not be a source location for a generated statement, the computed
value might not have been computed in the source at any point even.  Consider
re-association of an expression and then a re-associated part becoming
partially redundant.

 if ()
   tem = a + b;
 tem2 = a + c  + b;

after re-assoc + PRE:

 if ()
   tem = a + b;
else
   tem' = a + b;  // which sloc here?
tem'' = PHI <tem, tem'>  //  or here?  on the args?
tem2 = tem'' + c;

so what source location would you use for the inserted expression?  If
UNKNOWN is not
correct here then I think we need an explicit NOWHERE?

>> Fixing the location when the statement is inserted will reduce this
>> randomness.  I'm not sure I understand why you think this will break
>> coverage.  The examples given in this thread were helped by this
>> location fixing.
>
> What do you call randomness exactly?  GDB jumpiness?  I'm a little wary of
> fixing GDB jumpiness by distorting the debug info.  Ian has clearly outlined
> the correct approach to addressing it.
>
> Note that I didn't specifically reply to Dehao's patch here, which might be
> acceptable in the end, only to David's seemingly general statement about the
> "natural way" of putting a location on these statements.

Richard.

> --
> Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Xinliang David Li]

On Sun, Nov 25, 2012 at 4:40 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Nov 15, 2012 at 5:46 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> But UNKNOWN_LOCATION is effectively wrong as well.  If other
>>> optimizations move the statements above the inserted instruction, then
>>> the new instruction ends up inheriting whatever location happens to be
>>> in the previous statement.
>>
>> That's correct, UNKNOWN_LOCATION isn't a panacea either, although it can help
>> in some cases.  The only safe thing to do is to put the exact source location
>> of the statement, i.e. the location of the source construct for which the
>> statement has been generated.
>
> There may not be a source location for a generated statement, the computed
> value might not have been computed in the source at any point even.  Consider
> re-association of an expression and then a re-associated part becoming
> partially redundant.
>
>  if ()
>    tem = a + b;
>  tem2 = a + c  + b;
>
> after re-assoc + PRE:
>
>  if ()
>    tem = a + b;
> else
>    tem' = a + b;  // which sloc here?

If there was a else branch, the proposed patch by Dehao will use the
source location of the previous stmt 'tem' = a + b' was inserted
after.

I did not see any special source location handling in
tree-ssa-reassoc.c -- looks like the same principle is used there --
the new assignment after the operand shuffling will inherit the source
location of the placeholder gimple stmt.


> tem'' = PHI <tem, tem'>  //  or here?  on the args?
> tem2 = tem'' + c;
>
> so what source location would you use for the inserted expression?  If
> UNKNOWN is not
> correct here then I think we need an explicit NOWHERE?

Can that break line coverage info too, say, when source location get
stripped from reassociated expressions?

David




>
>>> Fixing the location when the statement is inserted will reduce this
>>> randomness.  I'm not sure I understand why you think this will break
>>> coverage.  The examples given in this thread were helped by this
>>> location fixing.
>>
>> What do you call randomness exactly?  GDB jumpiness?  I'm a little wary of
>> fixing GDB jumpiness by distorting the debug info.  Ian has clearly outlined
>> the correct approach to addressing it.
>>
>> Note that I didn't specifically reply to Dehao's patch here, which might be
>> acceptable in the end, only to David's seemingly general statement about the
>> "natural way" of putting a location on these statements.
>
> Richard.
>
>> --
>> Eric Botcazou

Re: [PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

On Sun, Nov 25, 2012 at 11:37 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Sun, Nov 25, 2012 at 4:40 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Nov 15, 2012 at 5:46 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>>> But UNKNOWN_LOCATION is effectively wrong as well.  If other
>>>> optimizations move the statements above the inserted instruction, then
>>>> the new instruction ends up inheriting whatever location happens to be
>>>> in the previous statement.
>>>
>>> That's correct, UNKNOWN_LOCATION isn't a panacea either, although it can help
>>> in some cases.  The only safe thing to do is to put the exact source location
>>> of the statement, i.e. the location of the source construct for which the
>>> statement has been generated.
>>
>> There may not be a source location for a generated statement, the computed
>> value might not have been computed in the source at any point even.  Consider
>> re-association of an expression and then a re-associated part becoming
>> partially redundant.
>>
>>  if ()
>>    tem = a + b;
>>  tem2 = a + c  + b;
>>
>> after re-assoc + PRE:
>>
>>  if ()
>>    tem = a + b;
>> else
>>    tem' = a + b;  // which sloc here?

For inserted expr/stmt, inheriting location from placeholder is making
sense. But when the placeholder has no location in the first place,
explicit NOWHERE is needed, otherwise it will just has random location
from other basic block.

Thus for this cases, sloc here should be NOWHERE.

However, in the unittest I provided:

  if (x > 0)
    ret += *a;
  else
    a++;
  ret += *a;

After PRE:

if (x > 0) {
  pretmp_1 = *a;
  ret += pretmp_1;
} else {
  a++;
  pretmp_2 = *a; // it should inherit location from prev line
}
pretmp_3 = PHI(pretmp_1, pretmp_2)

How does that sound?

Thanks,
Dehao

>
> If there was a else branch, the proposed patch by Dehao will use the
> source location of the previous stmt 'tem' = a + b' was inserted
> after.
>
> I did not see any special source location handling in
> tree-ssa-reassoc.c -- looks like the same principle is used there --
> the new assignment after the operand shuffling will inherit the source
> location of the placeholder gimple stmt.
>
>
>> tem'' = PHI <tem, tem'>  //  or here?  on the args?
>> tem2 = tem'' + c;
>>
>> so what source location would you use for the inserted expression?  If
>> UNKNOWN is not
>> correct here then I think we need an explicit NOWHERE?



>
> Can that break line coverage info too, say, when source location get
> stripped from reassociated expressions?
>
> David
>
>
>
>
>>
>>>> Fixing the location when the statement is inserted will reduce this
>>>> randomness.  I'm not sure I understand why you think this will break
>>>> coverage.  The examples given in this thread were helped by this
>>>> location fixing.
>>>
>>> What do you call randomness exactly?  GDB jumpiness?  I'm a little wary of
>>> fixing GDB jumpiness by distorting the debug info.  Ian has clearly outlined
>>> the correct approach to addressing it.
>>>
>>> Note that I didn't specifically reply to Dehao's patch here, which might be
>>> acceptable in the end, only to David's seemingly general statement about the
>>> "natural way" of putting a location on these statements.
>>
>> Richard.
>>
>>> --
>>> Eric Botcazou