[PATCH] Fix wrong register allocation for EH

[PATCH] Fix wrong register allocation for EH

[Eric Botcazou]

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

Hi,

the problem is responsible for ACATS and C++ failures on SPARC/Solaris,
for example g++.old-deja/g++.eh/inline2.C.  IRA doesn't correctly deal
with EH_RETURN_DATA_REGNO, unlike the old RA:

struct A {
  ~A()
  {
    try { throw 1; }
    catch (...) { }
  }
};

int main ()
{
  try
    {
      A a;
      throw 42;
    }
  catch (int i)
    {
      return (i != 42);
    }
}

When the first try/catch block is inlined into the second catch, the incoming
EH_RETURN_DATA_REGNO values for the second throw must be preserved while the 
first throw is being expanded and clobbers the EH_RETURN_DATA_REGNO registers.

These values are saved into pseudos but IRA can assign EH_RETURN_DATA_REGNO 
registers to these pseudos across EH edges, which defeats the purpose of 
saving the values of EH_RETURN_DATA_REGNO altogether.

The old RA has explicit couter-measures for such "problematic" cases:

#ifdef EH_RETURN_DATA_REGNO
      if (bb_has_eh_pred (bb))
	{
	  unsigned int i;
    
	  for (i = 0; ; ++i)
	    {
	      unsigned int regno = EH_RETURN_DATA_REGNO (i);
	      if (regno == INVALID_REGNUM)
		break;
	      record_one_conflict (allocnos_live, &hard_regs_live, regno);
	    }

	  EXECUTE_IF_SET_IN_SPARSESET (allocnos_live, i)
	    {
	      allocno[i].no_eh_reg = 1;
	    }
	}
#endif

      if (bb_has_abnormal_pred (bb))
	{
	  unsigned int i;
#ifdef STACK_REGS
	  /* Pseudos can't go in stack regs at the start of a basic block that
	     is reached by an abnormal edge. Likewise for call clobbered regs,
	     because caller-save, fixup_abnormal_edges and possibly the table
	     driven EH machinery are not quite ready to handle such regs live
	     across such edges.  */
	  EXECUTE_IF_SET_IN_SPARSESET (allocnos_live, i)
	    {
	      allocno[i].no_stack_reg = 1;
	    }

	  for (i = FIRST_STACK_REG; i <= LAST_STACK_REG; i++)
	    record_one_conflict (allocnos_live, &hard_regs_live, i);
#endif
	  
	  /* No need to record conflicts for call clobbered regs if we have
	     nonlocal labels around, as we don't ever try to allocate such
	     regs in this case.  */
	  if (! cfun->has_nonlocal_label)
	    for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
	      if (call_used_regs [i])
		record_one_conflict (allocnos_live, &hard_regs_live, i);
	}

but, while the second part (for abnormal edges) has been transposed in 
process_bb_node_lives, the first part (for EH edges) hasn't.

Hence the attached patch, tested on SPARC/Solaris, OK for mainline?


2008-11-10  Eric Botcazou  <ebotcazou@adacore.com>

	* ira-lives.c (process_bb_node_lives): Deal with EH_RETURN_DATA_REGNO.
	

-- 
Eric Botcazou

[-- Attachment #2: eh_return_data.diff --]
[-- Type: text/x-diff, Size: 1220 bytes --]

Index: ira-lives.c
===================================================================
--- ira-lives.c	(revision 141707)
+++ ira-lives.c	(working copy)
@@ -785,8 +785,6 @@ process_bb_node_lives (ira_loop_tree_nod
   unsigned int j;
   basic_block bb;
   rtx insn;
-  edge e;
-  edge_iterator ei;
   bitmap_iterator bi;
   bitmap reg_live_out;
   unsigned int px;
@@ -985,16 +983,23 @@ process_bb_node_lives (ira_loop_tree_nod
 	  curr_point++;
 	}
 
+#ifdef EH_RETURN_DATA_REGNO
+      if (bb_has_eh_pred (bb))
+	for (j = 0; ; ++j)
+	  {
+	    unsigned int regno = EH_RETURN_DATA_REGNO (j);
+	    if (regno == INVALID_REGNUM)
+	      break;
+	    make_regno_born (regno);
+	  }
+#endif
+
       /* Allocnos can't go in stack regs at the start of a basic block
 	 that is reached by an abnormal edge. Likewise for call
 	 clobbered regs, because caller-save, fixup_abnormal_edges and
 	 possibly the table driven EH machinery are not quite ready to
 	 handle such allocnos live across such edges.  */
-      FOR_EACH_EDGE (e, ei, bb->preds)
-	if (e->flags & EDGE_ABNORMAL)
-	  break;
-
-      if (e != NULL)
+      if (bb_has_abnormal_pred (bb))
 	{
 #ifdef STACK_REGS
 	  EXECUTE_IF_SET_IN_SPARSESET (allocnos_live, px)

Re: [PATCH] Fix wrong register allocation for EH

[Vladimir Makarov]

Eric Botcazou wrote:
>
> but, while the second part (for abnormal edges) has been transposed in 
> process_bb_node_lives, the first part (for EH edges) hasn't.
>
It looks like the code was removed by the patch

2008-09-04  Richard Sandiford  <rdsandiford@googlemail.com>

    PR middle-end/37243
    * ira-build.c (form_loop_tree): Reverse BB walk.
    (create_bb_allocnos): Likewise.
    * ira-lives.c (make_regno_born_and_dead, regs_set): Delete.
    (mark_reg_store): Rename to...
    (mark_ref_live): ...this and take a df_ref argument instead of
    note_stores arguments.  Assert that we have a register.
    (mark_reg_clobber): Delete.
    (def_conflicts_with_inputs_p): New function.
    (mark_reg_conflicts): Delete.
    (mark_reg_death): Rename to...
    (mark_ref_dead): ...this and take a df_ref argument instead of
    a register.  Assert that we have a register.
    (process_bb_node_lives): Hoist frequency calculation out of
    instruction walk.  Convert from a forwards scan to a backwards scan.
    Use DF_REF_USES and DF_REF_DEFS instead of register notes and
    note_stores.  Remove EH_RETURN_DATA_REGNO and regs_set handling.
    (create_allocno_live_ranges): Don't create regs_set.

I overlooked that when I was reviewing the patch.  My bad.
> Hence the attached patch, tested on SPARC/Solaris, OK for mainline?
>
Yes, Eric.  Thank you for the patch.
>
> 2008-11-10  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* ira-lives.c (process_bb_node_lives): Deal with EH_RETURN_DATA_REGNO.
> 	
>

Re: [PATCH] Fix wrong register allocation for EH

[Laurent GUERBY]

With your patch in the number of FAIL for ACATS on sparc-linux
goes down to only three :).

http://gcc.gnu.org/ml/gcc-testresults/2008-11/msg01191.html

FAIL:	cc70a01
FAIL:	cd1c04e
FAIL:	cxa4016

Compared to 8 before:

http://gcc.gnu.org/ml/gcc-testresults/2008-11/msg00710.html

I didn't spot recent sparc-solaris Ada testresults to compare 
with this time.

Please find below a quick analysis of the three fails, it looks like
two are still linked to exception handling, including one that fails
at -O0.

Let me know if you want me to open PRs.

Laurent

,.,. CC70A01 ACATS 2.5 08-11-14 15:14:56
---- CC70A01 Check that the visible part of a generic formal package
                includes the first list of basic declarative items of
                the package specification. Check for a generic package
                where formal package has (<>) actual part.
/home/guerby/trunk/gcc/testsuite/ada/acats/run_all.sh: line 15: 17163 Erreur de segmentation  $*
FAIL:	cc70a01

cc70a01 works at -O0 and -O1 and fails at -O2. At execution at -O2 I get under gdb:

$ gdb ./cc70a01
(gdb) r
Starting program: /home/guerby/tmp/cc70a01 

,.,. CC70A01 ACATS 2.5 08-11-14 16:42:49
---- CC70A01 Check that the visible part of a generic formal package
                includes the first list of basic declarative items of
                the package specification. Check for a generic package
                where formal package has (<>) actual part.

Program received signal SIGSEGV, Segmentation fault.
uw_update_context_1 (context=0xffa31018, fs=0xffa30cb0) at ../../../trunk/libgcc/../gcc/unwind-dw2.c:177
177	    return * (_Unwind_Ptr *) ptr;
Current language:  auto; currently c
(gdb) bt
#0  uw_update_context_1 (context=0xffa31018, fs=0xffa30cb0) at ../../../trunk/libgcc/../gcc/unwind-dw2.c:177
#1  0x00032c1c in uw_update_context (context=0xffa31018, fs=0xffa30cb0) at ../../../trunk/libgcc/../gcc/unwind-dw2.c:1388
#2  0x00033c84 in _Unwind_RaiseException (exc=0x5a038) at ../../../trunk/libgcc/../gcc/unwind.inc:127
#3  0x000175f4 in ada.exceptions.exception_propagation.propagate_exception (e=0x54840, from_signal_handler=false) at a-exexpr.adb:589
#4  0x00018364 in <__gnat_raise_nodefer_with_msg> (e=0x54840) at a-except.adb:829
#5  0x00018400 in <__gnat_raise_exception> (e=0x54840, message={P_ARRAY = 0xffa30bd0, P_BOUNDS = 0x36098}) at a-except.adb:868
#6  0x00013604 in cc70a01.my_matrix_package."*" (left={P_ARRAY = 0x40000000, P_BOUNDS = 0x3603c}, right={P_ARRAY = 0xffa31780, P_BOUNDS = 0x3602c}) at cc70a01_0.adb:51
#7  0x00013e18 in cc70a01 () at cc70a01.adb:55
(gdb) info local
orig_context = {reg = {0x0 <repeats 14 times>, 0xffa30bcc, 0xffa3159c, 0x0, 0x4, 0x8, 0xc, 0x10, 0x14, 0x18, 0x1c, 0x20, 0x24, 0x28, 0x2c, 0x30, 0x34, 0x38, 0x3c, 0x0 <repeats 71 times>}, cfa = 0x0, 
  ra = 0x13604, lsda = 0x416ef, bases = {tbase = 0x0, dbase = 0x0, func = 0x1347c}, flags = 1073741824, version = 0, args_size = 0, by_value = '\0' <repeats 102 times>}
cfa = <value optimized out>
tmp_sp = {ptr = 0, word = 0}
(gdb)


,.,. CXA4016 ACATS 2.5 08-11-14 15:44:39
---- CXA4016 Check that the subprograms defined in package
                Ada.Strings.Wide_Fixed are available, and that they
                produce correct results.
/home/guerby/trunk/gcc/testsuite/ada/acats/run_all.sh: line 15:  3111 Erreur de segmentation  $*
FAIL:	cxa4016

This one fails at -O0 and above, the backtrace is similar to cc70a01:

(gdb) r
Starting program: /home/guerby/tmp/cxa4016 

,.,. CXA4016 ACATS 2.5 08-11-14 16:48:57
---- CXA4016 Check that the subprograms defined in package
                Ada.Strings.Wide_Fixed are available, and that they
                produce correct results.

Program received signal SIGSEGV, Segmentation fault.
uw_update_context_1 (context=0xffab41f0, fs=0xffab3e88) at ../../../trunk/libgcc/../gcc/unwind-dw2.c:177
177	    return * (_Unwind_Ptr *) ptr;
Current language:  auto; currently c
(gdb) bt
#0  uw_update_context_1 (context=0xffab41f0, fs=0xffab3e88) at ../../../trunk/libgcc/../gcc/unwind-dw2.c:177
#1  0x000441b8 in uw_update_context (context=0xffab41f0, fs=0xffab3e88) at ../../../trunk/libgcc/../gcc/unwind-dw2.c:1388
#2  0x00045220 in _Unwind_RaiseException (exc=0x6e098) at ../../../trunk/libgcc/../gcc/unwind.inc:127
#3  0x00021df0 in ada.exceptions.exception_propagation.propagate_exception (e=0x68994, from_signal_handler=false) at a-exexpr.adb:589
#4  0x00022b60 in <__gnat_raise_nodefer_with_msg> (e=0x68994) at a-except.adb:829
#5  0x00022bfc in <__gnat_raise_exception> (e=0x68994, message={P_ARRAY = 0xffab3da8, P_BOUNDS = 0x49d80}) at a-except.adb:868
#6  0x00025b88 in ada.strings.wide_fixed.replace_slice (source={P_ARRAY = 0xffab52c4, P_BOUNDS = 0xffab4908}, low=12, high=10, by={P_ARRAY = 0x47ed8, P_BOUNDS = 0xffab4900}) at a-stwifi.adb:447
#7  0x00026934 in ada.strings.wide_fixed.replace_slice (source={P_ARRAY = 0xffab52c4, P_BOUNDS = 0x47ed0}, low=12, high=10, by={P_ARRAY = 0x47ed8, P_BOUNDS = 0x47ee8}, drop=ada__strings__error, 
    justify=ada__strings__left, pad=32 ' ') at a-stwifi.adb:480
#8  0x00017e64 in cxa4016 () at cxa4016.adb:131
(gdb) info local
orig_context = {reg = {0x0 <repeats 14 times>, 0xffab3da4, 0xffab4774, 0x0, 0x4, 0x8, 0xc, 0x10, 0x14, 0x18, 0x1c, 0x20, 0x24, 0x28, 0x2c, 0x30, 0x34, 0x38, 0x3c, 0x0 <repeats 71 times>}, cfa = 0x0, 
  ra = 0x25b88, lsda = 0x55aff, bases = {tbase = 0x0, dbase = 0x0, func = 0x25968}, flags = 1073741824, version = 0, args_size = 0, by_value = '\0' <repeats 102 times>}
cfa = <value optimized out>
tmp_sp = {ptr = 0, word = 0}

,.,. CD1C04E ACATS 2.5 08-11-14 15:16:58
---- CD1C04E CHECK THAT A RECORD REPRESENTATION CLAUSE CAN BE GIVEN FOR 
                A DERIVED RECORD TYPE EVEN IF THE REPRESENTATION IS
                INHERITED FROM THE PARENT, AND THAT THE REPRESENTATION
                CLAUSE FOR THE DERIVED TYPE OVERRIDES THAT OF THE PARENT
                TYPE.
   * CD1C04E THE VALUES OF DERIVED_TYPE COMPONENTS WERE INCORRECT.
**** CD1C04E FAILED ****************************.
FAIL:	cd1c04e

cd1c04e pass at -O0 and starts failing at -O1.



On Mon, 2008-11-10 at 23:56 +0100, Eric Botcazou wrote:
> Hi,
> 
> the problem is responsible for ACATS and C++ failures on SPARC/Solaris,
> for example g++.old-deja/g++.eh/inline2.C.  IRA doesn't correctly deal
> with EH_RETURN_DATA_REGNO, unlike the old RA:
...
> 2008-11-10  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	* ira-lives.c (process_bb_node_lives): Deal with EH_RETURN_DATA_REGNO.
> 	
> 

Re: [PATCH] Fix wrong register allocation for EH

[Laurent GUERBY]

On Fri, 2008-11-14 at 17:53 +0100, Laurent GUERBY wrote:
> ,.,. CD1C04E ACATS 2.5 08-11-14 15:16:58
> ---- CD1C04E CHECK THAT A RECORD REPRESENTATION CLAUSE CAN BE GIVEN FOR 
>                 A DERIVED RECORD TYPE EVEN IF THE REPRESENTATION IS
>                 INHERITED FROM THE PARENT, AND THAT THE REPRESENTATION
>                 CLAUSE FOR THE DERIVED TYPE OVERRIDES THAT OF THE PARENT
>                 TYPE.
>    * CD1C04E THE VALUES OF DERIVED_TYPE COMPONENTS WERE INCORRECT.
> **** CD1C04E FAILED ****************************.
> FAIL:	cd1c04e
> 
> cd1c04e pass at -O0 and starts failing at -O1.

This one also fails on hppa (where it is the only FAIL):

http://gcc.gnu.org/ml/gcc-testresults/2008-11/msg01048.html

Laurent

Re: [PATCH] Fix wrong register allocation for EH

[Eric Botcazou]

:> > ,.,. CD1C04E ACATS 2.5 08-11-14 15:16:58
> > ---- CD1C04E CHECK THAT A RECORD REPRESENTATION CLAUSE CAN BE GIVEN FOR
> >                 A DERIVED RECORD TYPE EVEN IF THE REPRESENTATION IS
> >                 INHERITED FROM THE PARENT, AND THAT THE REPRESENTATION
> >                 CLAUSE FOR THE DERIVED TYPE OVERRIDES THAT OF THE PARENT
> >                 TYPE.
> >    * CD1C04E THE VALUES OF DERIVED_TYPE COMPONENTS WERE INCORRECT.
> > **** CD1C04E FAILED ****************************.
> > FAIL:	cd1c04e
> >
> > cd1c04e pass at -O0 and starts failing at -O1.
>
> This one also fails on hppa (where it is the only FAIL):

That's the only FAIL on SPARC/Solaris and SPARC64/Solaris as well.  I'll look 
into it but I cannot do much for the 2 other failures on SPARC/Linux so you 
could indeed open a PR for them.

-- 
Eric Botcazou

Re: [PATCH] Fix wrong register allocation for EH

[Richard Sandiford]

Vladimir Makarov <vmakarov@redhat.com> writes:
> Eric Botcazou wrote:
>>
>> but, while the second part (for abnormal edges) has been transposed in 
>> process_bb_node_lives, the first part (for EH edges) hasn't.
>>
> It looks like the code was removed by the patch
>
> 2008-09-04  Richard Sandiford  <rdsandiford@googlemail.com>
>
>     PR middle-end/37243
>     * ira-build.c (form_loop_tree): Reverse BB walk.
>     (create_bb_allocnos): Likewise.
>     * ira-lives.c (make_regno_born_and_dead, regs_set): Delete.
>     (mark_reg_store): Rename to...
>     (mark_ref_live): ...this and take a df_ref argument instead of
>     note_stores arguments.  Assert that we have a register.
>     (mark_reg_clobber): Delete.
>     (def_conflicts_with_inputs_p): New function.
>     (mark_reg_conflicts): Delete.
>     (mark_reg_death): Rename to...
>     (mark_ref_dead): ...this and take a df_ref argument instead of
>     a register.  Assert that we have a register.
>     (process_bb_node_lives): Hoist frequency calculation out of
>     instruction walk.  Convert from a forwards scan to a backwards scan.
>     Use DF_REF_USES and DF_REF_DEFS instead of register notes and
>     note_stores.  Remove EH_RETURN_DATA_REGNO and regs_set handling.
>     (create_allocno_live_ranges): Don't create regs_set.
>
> I overlooked that when I was reviewing the patch.  My bad.

Sorry for the screw-up.  I'd thought this was one of the things that DF
would handle for us.

Richard

Re: [PATCH] Fix wrong register allocation for EH

[Eric Botcazou]

> Sorry for the screw-up.  I'd thought this was one of the things that DF
> would handle for us.

The problem is that, while DF correctly detects them as artificial defs at the 
beginning of the BB, the info is not propagated to the RA.  So you would need 
an explicit walk over the result of df_get_artificial_defs, but I think that 
mimicing what is done in global_conflicts is more consistent.

-- 
Eric Botcazou

Re: [PATCH] Fix wrong register allocation for EH

[Laurent GUERBY]

On Fri, 2008-11-14 at 20:17 +0100, Eric Botcazou wrote:
> That's the only FAIL on SPARC/Solaris and SPARC64/Solaris as well.  I'll look 
> into it but I cannot do much for the 2 other failures on SPARC/Linux so you 
> could indeed open a PR for them.

Done:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38131

Laurent

Re: [PATCH] Fix wrong register allocation for EH

[Vladimir Makarov]

Richard Sandiford wrote:
>
>
> Sorry for the screw-up.  I'd thought this was one of the things that DF
> would handle for us.
>
>   
Please don't worry, Richard.  I thought this too. I really appreciate 
your help and work on IRA.  I really mean it.