Small improvements to coverage info (1/n)

Message ID 4824451.YNsNmM2r07@polaris
State New
Headers show
Series
  • Small improvements to coverage info (1/n)
Related show

Commit Message

Eric Botcazou July 3, 2019, 10:46 a.m.
Hi,

we have collected a number of small improvements to coverage info generated by 
the compiler over the years.  One of the issues is when a new expression or 
statement is built without source location information and ends up inheriting 
the source location information of the previous instruction in the debug info, 
which can be totally unrelated.

Tested on x86_64-suse-linux, both GCC and GDB, OK for mainline?


2019-07-03  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-cfg.c (gimple_make_forwarder_block): Propagate location info on
	phi nodes if possible.
	* tree-scalar-evolution.c (final_value_replacement_loop): Propagate
	location info on the newly created statement.
	* tree-ssa-loop-manip.c (create_iv): Propagate location info on the
	newly created increment if needed.

-- 
Eric Botcazou

Comments

Jeff Law July 3, 2019, 9:58 p.m. | #1
On 7/3/19 4:46 AM, Eric Botcazou wrote:
> Hi,

> 

> we have collected a number of small improvements to coverage info generated by 

> the compiler over the years.  One of the issues is when a new expression or 

> statement is built without source location information and ends up inheriting 

> the source location information of the previous instruction in the debug info, 

> which can be totally unrelated.

> 

> Tested on x86_64-suse-linux, both GCC and GDB, OK for mainline?

> 

> 

> 2019-07-03  Eric Botcazou  <ebotcazou@adacore.com>

> 

> 	* tree-cfg.c (gimple_make_forwarder_block): Propagate location info on

> 	phi nodes if possible.

> 	* tree-scalar-evolution.c (final_value_replacement_loop): Propagate

> 	location info on the newly created statement.

> 	* tree-ssa-loop-manip.c (create_iv): Propagate location info on the

> 	newly created increment if needed.

> 

OK
jeff
Jakub Jelinek July 5, 2019, 12:12 p.m. | #2
On Wed, Jul 03, 2019 at 12:46:37PM +0200, Eric Botcazou wrote:
> 2019-07-03  Eric Botcazou  <ebotcazou@adacore.com>

> 

> 	* tree-cfg.c (gimple_make_forwarder_block): Propagate location info on

> 	phi nodes if possible.

> 	* tree-scalar-evolution.c (final_value_replacement_loop): Propagate

> 	location info on the newly created statement.

> 	* tree-ssa-loop-manip.c (create_iv): Propagate location info on the

> 	newly created increment if needed.


> --- tree-ssa-loop-manip.c	(revision 272930)

> +++ tree-ssa-loop-manip.c	(working copy)

> @@ -126,10 +126,22 @@ create_iv (tree base, tree step, tree va

>      gsi_insert_seq_on_edge_immediate (pe, stmts);

>  

>    stmt = gimple_build_assign (va, incr_op, vb, step);

> +  /* Prevent the increment from inheriting a bogus location if it is not put

> +     immediately after a statement whose location is known.  */

>    if (after)

> -    gsi_insert_after (incr_pos, stmt, GSI_NEW_STMT);

> +    {

> +      if (gsi_end_p (*incr_pos))

> +	{

> +	  edge e = single_succ_edge (gsi_bb (*incr_pos));

> +	  gimple_set_location (stmt, e->goto_locus);

> +	}

> +      gsi_insert_after (incr_pos, stmt, GSI_NEW_STMT);

> +    }

>    else

> -    gsi_insert_before (incr_pos, stmt, GSI_NEW_STMT);

> +    {

> +      gimple_set_location (stmt, gimple_location (gsi_stmt (*incr_pos)));

> +      gsi_insert_before (incr_pos, stmt, GSI_NEW_STMT);

> +    }


This change broke gomp/pr88107.c test:
FAIL: gcc.dg/gomp/pr88107.c (internal compiler error)
FAIL: gcc.dg/gomp/pr88107.c (test for excess errors)
Excess errors:
during GIMPLE pass: graphite
/usr/src/gcc/gcc/testsuite/gcc.dg/gomp/pr88107.c:26:1: internal compiler error: Segmentation fault
0x11942a4 crash_signal
        ../../gcc/toplev.c:326
0x13b5861 gimple_location
        ../../gcc/gimple.h:1805
0x13b76f8 create_iv(tree_node*, tree_node*, tree_node*, loop*, gimple_stmt_iterator*, bool, tree_node**, tree_node**)
        ../../gcc/tree-ssa-loop-manip.c:142
0xaa2c95 create_empty_loop_on_edge(edge_def*, tree_node*, tree_node*, tree_node*, tree_node*, tree_node**, tree_node**, loop*)
        ../../gcc/cfgloopmanip.c:831

Apparently gsi_end_p (*incr_pos) is true and after is false, so
gsi_stmt (*incr_pos) is NULL.  One needs graphite enabled.

	Jakub
Jeff Law July 5, 2019, 2:32 p.m. | #3
On 7/5/19 6:12 AM, Jakub Jelinek wrote:
> On Wed, Jul 03, 2019 at 12:46:37PM +0200, Eric Botcazou wrote:

>> 2019-07-03  Eric Botcazou  <ebotcazou@adacore.com>

>>

>> 	* tree-cfg.c (gimple_make_forwarder_block): Propagate location info on

>> 	phi nodes if possible.

>> 	* tree-scalar-evolution.c (final_value_replacement_loop): Propagate

>> 	location info on the newly created statement.

>> 	* tree-ssa-loop-manip.c (create_iv): Propagate location info on the

>> 	newly created increment if needed.

> 

>> --- tree-ssa-loop-manip.c	(revision 272930)

>> +++ tree-ssa-loop-manip.c	(working copy)

>> @@ -126,10 +126,22 @@ create_iv (tree base, tree step, tree va

>>      gsi_insert_seq_on_edge_immediate (pe, stmts);

>>  

>>    stmt = gimple_build_assign (va, incr_op, vb, step);

>> +  /* Prevent the increment from inheriting a bogus location if it is not put

>> +     immediately after a statement whose location is known.  */

>>    if (after)

>> -    gsi_insert_after (incr_pos, stmt, GSI_NEW_STMT);

>> +    {

>> +      if (gsi_end_p (*incr_pos))

>> +	{

>> +	  edge e = single_succ_edge (gsi_bb (*incr_pos));

>> +	  gimple_set_location (stmt, e->goto_locus);

>> +	}

>> +      gsi_insert_after (incr_pos, stmt, GSI_NEW_STMT);

>> +    }

>>    else

>> -    gsi_insert_before (incr_pos, stmt, GSI_NEW_STMT);

>> +    {

>> +      gimple_set_location (stmt, gimple_location (gsi_stmt (*incr_pos)));

>> +      gsi_insert_before (incr_pos, stmt, GSI_NEW_STMT);

>> +    }

> 

> This change broke gomp/pr88107.c test:

> FAIL: gcc.dg/gomp/pr88107.c (internal compiler error)

> FAIL: gcc.dg/gomp/pr88107.c (test for excess errors)

> Excess errors:

> during GIMPLE pass: graphite

> /usr/src/gcc/gcc/testsuite/gcc.dg/gomp/pr88107.c:26:1: internal compiler error: Segmentation fault

> 0x11942a4 crash_signal

>         ../../gcc/toplev.c:326

> 0x13b5861 gimple_location

>         ../../gcc/gimple.h:1805

> 0x13b76f8 create_iv(tree_node*, tree_node*, tree_node*, loop*, gimple_stmt_iterator*, bool, tree_node**, tree_node**)

>         ../../gcc/tree-ssa-loop-manip.c:142

> 0xaa2c95 create_empty_loop_on_edge(edge_def*, tree_node*, tree_node*, tree_node*, tree_node*, tree_node**, tree_node**, loop*)

>         ../../gcc/cfgloopmanip.c:831

> 

> Apparently gsi_end_p (*incr_pos) is true and after is false, so

> gsi_stmt (*incr_pos) is NULL.  One needs graphite enabled.

Which might explain the massive failures I'm seeing with graphite
enabled on various *-elf targets.

ft32-elf for example:
> Tests that now fail, but worked before (34 tests):

> 

> ft32-sim: gcc.dg/graphite/id-10.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/id-16.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/id-25.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/id-pr46834.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/id-pr47046.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/id-pr48805.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/interchange-8.c scan-tree-dump graphite "tiled by"

> ft32-sim: gcc.dg/graphite/pr29330.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr30565.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr31183.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr33576.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr36287.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr42211.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr42914.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr42917.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr46168.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr46966.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr68428.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr68493.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr68756.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr69068.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr69292.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr70045.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr77362.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr80906.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr80906.c scan-tree-dump graphite "isl AST to Gimple succeeded"

> ft32-sim: gcc.dg/graphite/pr81373-2.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr81373.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr82421.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr83255.c scan-tree-dump graphite "loop nest optimized"

> ft32-sim: gcc.dg/graphite/pr84204.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr84205.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr85935.c (test for excess errors)

> ft32-sim: gcc.dg/graphite/pr87931.c (test for excess errors)



SImilarly for c6x, arc, & nds32 (everything that's run in the last few
hours).

I suspect there'll be more as the day goes on.

jeff
Eric Botcazou July 5, 2019, 5:38 p.m. | #4
> This change broke gomp/pr88107.c test:

> FAIL: gcc.dg/gomp/pr88107.c (internal compiler error)

> FAIL: gcc.dg/gomp/pr88107.c (test for excess errors)

> Excess errors:

> during GIMPLE pass: graphite

> /usr/src/gcc/gcc/testsuite/gcc.dg/gomp/pr88107.c:26:1: internal compiler

> error: Segmentation fault 0x11942a4 crash_signal

>         ../../gcc/toplev.c:326

> 0x13b5861 gimple_location

>         ../../gcc/gimple.h:1805

> 0x13b76f8 create_iv(tree_node*, tree_node*, tree_node*, loop*,

> gimple_stmt_iterator*, bool, tree_node**, tree_node**)

> ../../gcc/tree-ssa-loop-manip.c:142

> 0xaa2c95 create_empty_loop_on_edge(edge_def*, tree_node*, tree_node*,

> tree_node*, tree_node*, tree_node**, tree_node**, loop*)

> ../../gcc/cfgloopmanip.c:831

> 

> Apparently gsi_end_p (*incr_pos) is true and after is false, so

> gsi_stmt (*incr_pos) is NULL.  One needs graphite enabled.


OK, thanks for the heads up.  I have installed the attached fixlet for now.


	* tree-ssa-loop-manip.c (create_iv): Add missing guard for gsi_end_p.

-- 
Eric Botcazou
Index: tree-ssa-loop-manip.c
===================================================================
--- tree-ssa-loop-manip.c	(revision 273133)
+++ tree-ssa-loop-manip.c	(working copy)
@@ -139,7 +139,8 @@ create_iv (tree base, tree step, tree va
     }
   else
     {
-      gimple_set_location (stmt, gimple_location (gsi_stmt (*incr_pos)));
+      if (!gsi_end_p (*incr_pos))
+	gimple_set_location (stmt, gimple_location (gsi_stmt (*incr_pos)));
       gsi_insert_before (incr_pos, stmt, GSI_NEW_STMT);
     }

Patch

Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 272930)
+++ tree-cfg.c	(working copy)
@@ -5756,6 +5756,7 @@  gimple_make_forwarder_block (edge fallth
   basic_block dummy, bb;
   tree var;
   gphi_iterator gsi;
+  bool forward_location_p;
 
   dummy = fallthru->src;
   bb = fallthru->dest;
@@ -5763,6 +5764,9 @@  gimple_make_forwarder_block (edge fallth
   if (single_pred_p (bb))
     return;
 
+  /* We can forward location info if we have only one predecessor.  */
+  forward_location_p = single_pred_p (dummy);
+
   /* If we redirected a branch we must create new PHI nodes at the
      start of BB.  */
   for (gsi = gsi_start_phis (dummy); !gsi_end_p (gsi); gsi_next (&gsi))
@@ -5774,7 +5778,8 @@  gimple_make_forwarder_block (edge fallth
       new_phi = create_phi_node (var, bb);
       gimple_phi_set_result (phi, copy_ssa_name (var, phi));
       add_phi_arg (new_phi, gimple_phi_result (phi), fallthru,
-		   UNKNOWN_LOCATION);
+		   forward_location_p
+		   ? gimple_phi_arg_location (phi, 0) : UNKNOWN_LOCATION);
     }
 
   /* Add the arguments we have stored on edges.  */
Index: tree-scalar-evolution.c
===================================================================
--- tree-scalar-evolution.c	(revision 272930)
+++ tree-scalar-evolution.c	(working copy)
@@ -3680,6 +3680,8 @@  final_value_replacement_loop (struct loo
 					true, GSI_SAME_STMT);
 
       gassign *ass = gimple_build_assign (rslt, def);
+      gimple_set_location (ass,
+			   gimple_phi_arg_location (phi, exit->dest_idx));
       gsi_insert_before (&gsi, ass, GSI_SAME_STMT);
       if (dump_file)
 	{
Index: tree-ssa-loop-manip.c
===================================================================
--- tree-ssa-loop-manip.c	(revision 272930)
+++ tree-ssa-loop-manip.c	(working copy)
@@ -126,10 +126,22 @@  create_iv (tree base, tree step, tree va
     gsi_insert_seq_on_edge_immediate (pe, stmts);
 
   stmt = gimple_build_assign (va, incr_op, vb, step);
+  /* Prevent the increment from inheriting a bogus location if it is not put
+     immediately after a statement whose location is known.  */
   if (after)
-    gsi_insert_after (incr_pos, stmt, GSI_NEW_STMT);
+    {
+      if (gsi_end_p (*incr_pos))
+	{
+	  edge e = single_succ_edge (gsi_bb (*incr_pos));
+	  gimple_set_location (stmt, e->goto_locus);
+	}
+      gsi_insert_after (incr_pos, stmt, GSI_NEW_STMT);
+    }
   else
-    gsi_insert_before (incr_pos, stmt, GSI_NEW_STMT);
+    {
+      gimple_set_location (stmt, gimple_location (gsi_stmt (*incr_pos)));
+      gsi_insert_before (incr_pos, stmt, GSI_NEW_STMT);
+    }
 
   initial = force_gimple_operand (base, &stmts, true, var);
   if (stmts)