Patchwork fix restore of r30 on eh_return for ppc-aix / -mminimal-toc

login
register
mail settings
Submitter Olivier Hainque
Date Sept. 23, 2010, 8:06 a.m.
Message ID <20100923080646.GA7817@cardhu.act-europe.fr>
Download mbox | patch
Permalink /patch/65508/
State New
Headers show

Comments

Olivier Hainque - Sept. 23, 2010, 8:06 a.m.
Hello,

Late experiments with -mminimal-toc on ppc-aix revealed a problem with
restores of r30 on exception propagation.

The register indeed is not saved/restored as it should in the sequence
triggered by __builtin_unwind_init/__builtin_eh_return, as visible from
compiling the testcase below:

   void foo ()
   {
     long l; void *p;
     volatile int x;

     __builtin_unwind_init ();
     x = 12;
     __builtin_eh_return (l, p);
   }
 
 cc1 -O2 -mminimal-toc foo.c

   .foo:
        ...
        stw 28,128(1)
        stw 29,132(1)
        stw 31,140(1)
        ...

To determine whether a register is to be saved/restores, the rs6000
back-end resorts to

   /* Determine whether the gp REG is really used.  */

   static bool
   rs6000_reg_live_or_pic_offset_p (int reg)
   {
     return ((df_regs_ever_live_p (reg)
	      && (!call_used_regs[reg]
		  || (reg == RS6000_PIC_OFFSET_TABLE_REGNUM
		      && TARGET_TOC && TARGET_MINIMAL_TOC)))
	     || (reg == RS6000_PIC_OFFSET_TABLE_REGNUM
		 && ((DEFAULT_ABI == ABI_V4 && flag_pic != 0)
		     || (DEFAULT_ABI == ABI_DARWIN && flag_pic))));
   }
 
which always checks for liveness on AIX.

__builtin_unwind_init extends the set of registers to include with:

  reload ()
  ...
  if (crtl->saves_all_registers)
    for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
      if (! call_used_regs[i] && ! fixed_regs[i] && ! LOCAL_REGNO (i))
        df_set_regs_ever_live (i, true);

but r30 isn't included in the set with -mminimal-toc because of

  rs6000_conditional_register_usage ()
  ...
  if (TARGET_TOC && TARGET_MINIMAL_TOC)
    fixed_regs[RS6000_PIC_OFFSET_TABLE_REGNUM]
      = call_used_regs[RS6000_PIC_OFFSET_TABLE_REGNUM] = 1;


The attached patch fixes this by adjusting the rs6000 predicate
to claim "really used" for eh_return all the registers that are checked
for liveness in the regular case.

Bootstrapped and regression tested on powerpc-ibm-aix5.3.0.

OK ?

Thanks in advance,

Olivier

2010-09-23  Olivier Hainque  <hainque@adacore.com>

	* config/rs6000/rs6000.c (rs6000_reg_live_or_pic_offset_p):
	If the current function calls eh_return, claim live all registers
	that we need to check for liveness otherwise.

	testsuite/
	* gcc.target/powerpc (ehreturn.c): New test.
Olivier Hainque - Oct. 6, 2010, 8:50 p.m.
Hello,

Ping for

  http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01838.html

Olivier Hainque wrote:
> [on powerpc-aix, with -mminimal-toc] ... r30 is not saved/restored
> as it should in the sequence triggered by __builtin_unwind_init / 
> __builtin_eh_return

Thanks much in advance,

With Kind Regards,

Olivier
Olivier Hainque - Oct. 18, 2010, 8:02 a.m.
Hello,

Ping # 2 for

  http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01838.html

Olivier Hainque wrote:
> [on powerpc-aix, with -mminimal-toc] ... r30 is not saved/restored
> as it should in the sequence triggered by __builtin_unwind_init /
> __builtin_eh_return

Thanks much in advance,

With Kind Regards,
 
Olivier
David Edelsohn - Oct. 18, 2010, 3:27 p.m.
On Mon, Oct 18, 2010 at 4:02 AM, Olivier Hainque <hainque@adacore.com> wrote:
> Hello,
>
> Ping # 2 for
>
>  http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01838.html
>
> Olivier Hainque wrote:
>> [on powerpc-aix, with -mminimal-toc] ... r30 is not saved/restored
>> as it should in the sequence triggered by __builtin_unwind_init /
>> __builtin_eh_return

Okay.

Thanks, David
Alan Modra - Oct. 19, 2010, 3:59 a.m.
On Wed, Oct 06, 2010 at 10:50:51PM +0200, Olivier Hainque wrote:
> Hello,
> 
> Ping for
> 
>   http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01838.html
> 
> Olivier Hainque wrote:
> > [on powerpc-aix, with -mminimal-toc] ... r30 is not saved/restored
> > as it should in the sequence triggered by __builtin_unwind_init / 
> > __builtin_eh_return

This affects powerpc64-linux too.  FWIW, the patch looks good to me
except for the testsuite addition, which needs some tweaks.  I think
you'll need to dg-skip-if darwin, add -mno-multiple to the flags, and
scan for "st\\[wd\\] 30," so that 64-bit targets pass.
Olivier Hainque - Oct. 20, 2010, 10:23 a.m.
Hello,

Alan Modra wrote:
> This affects powerpc64-linux too.  FWIW, the patch looks good to me
> except for the testsuite addition, which needs some tweaks.  I think
> you'll need to dg-skip-if darwin, add -mno-multiple to the flags, and
> scan for "st\\[wd\\] 30," so that 64-bit targets pass.

 OK. After David's approval (thanks :), I have installed with the test
 adjustments you suggested. I was not able to verify that they do have the
 intended effect for ppc64-linux.

 Thanks for your feedback,

 Olivier

Patch

--- config/rs6000/rs6000.c	(revision 163943)
+++ config/rs6000/rs6000.c	(working copy)
@@ -19584,8 +19591,13 @@ 
 static bool
 rs6000_reg_live_or_pic_offset_p (int reg)
 {
-  return ((df_regs_ever_live_p (reg)
+  /* If the function calls eh_return, claim used all the registers that would
+     be checked for liveness otherwise.  This is required for the PIC offset
+     register with -mminimal-toc on AIX, as it is advertised as "fixed" for
+     register allocation purposes in this case.  */
+
+  return (((crtl->calls_eh_return || df_regs_ever_live_p (reg))
            && (!call_used_regs[reg]
                || (reg == RS6000_PIC_OFFSET_TABLE_REGNUM
                    && TARGET_TOC && TARGET_MINIMAL_TOC)))
--- testsuite/gcc.target/powerpc/ehreturn.c	(revision 0)
+++ testsuite/gcc.target/powerpc/ehreturn.c	(revision 0)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -mminimal-toc" } */
+
+void foo ()
+{
+  long l; void *p;
+  volatile int x;
+
+  __builtin_unwind_init ();
+  x = 12;
+  __builtin_eh_return (l, p);
+}
+
+/* { dg-final { scan-assembler "stw 30," } } */