Patchwork [rs6000] Avoid clobbering stack pointer via P8 fusion peephole

login
register
mail settings
Submitter Ulrich Weigand
Date March 27, 2014, 6:49 p.m.
Message ID <201403271849.s2RIn7hS028947@d03av02.boulder.ibm.com>
Download mbox | patch
Permalink /patch/334452/
State New
Headers show

Comments

Ulrich Weigand - March 27, 2014, 6:49 p.m.
Hello,

when trying to build Ada for powerpc64le-linux, I ran into an ICE
in fixup_args_size_notes.

It turns out that the p8 fusion peephole acts on these two insns
from the epilog sequence:

(insn 1693 1078 1079 91 (set (reg:DI 7 7)
        (plus:DI (reg/f:DI 31 31)
            (const_int 65536 [0x10000]))) 82 {*adddi3_internal1}
     (nil))
(insn 1079 1693 1511 91 (set (reg/f:DI 1 1)
        (mem/c:DI (plus:DI (reg:DI 7 7)
                (const_int -16096 [0xffffffffffffc120])) [233 %sfp+49440 S8 A64])) 519 {*movdi_internal64}
     (expr_list:REG_DEAD (reg:DI 7 7)
        (expr_list:REG_ARGS_SIZE (const_int 0 [0])
            (nil))))

and replaces them by:

(insn 1776 1078 1777 91 (set (reg/f:DI 1 1)
        (plus:DI (reg/f:DI 31 31)          
            (const_int 65536 [0x10000]))) -1
     (nil))                                 

(insn 1777 1776 1511 91 (set (reg/f:DI 1 1)
        (mem/c:DI (plus:DI (reg/f:DI 1 1)  
                (const_int -16096 [0xffffffffffffc120])) [233  S8 A8])) -1
     (expr_list:REG_ARGS_SIZE (const_int 0 [0])                           
        (nil)))                                                           

Then peephole common code thinks it needs to re-create the REG_ARGS_SIZE
note and fails since the code is too complex for it to understand.  (Which
is reasonable since it doesn't know what value is being restored from the
stack here.)

However, the more fundamental problem seems to be that this transformation
should be invalid anyway, since it creates an intermediate state where
the stack pointer points to a location without proper backchain, which
violates the ABI.

The following patch fixes this by disabling the fusion peephole in those
cases where it would introduce a new use of the stack pointer as temporary
register.

Tested on powerpc64le-linux.  OK for mainline (and 4.8 after the big patch
series is committed)?

Bye,
Ulrich

ChangeLog:

	* config/rs6000/rs6000.c (fusion_gpr_load_p): Refuse optimization
	if it would clobber the stack pointer, even temporarily.
David Edelsohn - March 27, 2014, 9:33 p.m.
On Thu, Mar 27, 2014 at 2:49 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Hello,
>
> when trying to build Ada for powerpc64le-linux, I ran into an ICE
> in fixup_args_size_notes.
>
> It turns out that the p8 fusion peephole acts on these two insns
> from the epilog sequence:
>
> (insn 1693 1078 1079 91 (set (reg:DI 7 7)
>         (plus:DI (reg/f:DI 31 31)
>             (const_int 65536 [0x10000]))) 82 {*adddi3_internal1}
>      (nil))
> (insn 1079 1693 1511 91 (set (reg/f:DI 1 1)
>         (mem/c:DI (plus:DI (reg:DI 7 7)
>                 (const_int -16096 [0xffffffffffffc120])) [233 %sfp+49440 S8 A64])) 519 {*movdi_internal64}
>      (expr_list:REG_DEAD (reg:DI 7 7)
>         (expr_list:REG_ARGS_SIZE (const_int 0 [0])
>             (nil))))
>
> and replaces them by:
>
> (insn 1776 1078 1777 91 (set (reg/f:DI 1 1)
>         (plus:DI (reg/f:DI 31 31)
>             (const_int 65536 [0x10000]))) -1
>      (nil))
>
> (insn 1777 1776 1511 91 (set (reg/f:DI 1 1)
>         (mem/c:DI (plus:DI (reg/f:DI 1 1)
>                 (const_int -16096 [0xffffffffffffc120])) [233  S8 A8])) -1
>      (expr_list:REG_ARGS_SIZE (const_int 0 [0])
>         (nil)))
>
> Then peephole common code thinks it needs to re-create the REG_ARGS_SIZE
> note and fails since the code is too complex for it to understand.  (Which
> is reasonable since it doesn't know what value is being restored from the
> stack here.)
>
> However, the more fundamental problem seems to be that this transformation
> should be invalid anyway, since it creates an intermediate state where
> the stack pointer points to a location without proper backchain, which
> violates the ABI.
>
> The following patch fixes this by disabling the fusion peephole in those
> cases where it would introduce a new use of the stack pointer as temporary
> register.
>
> Tested on powerpc64le-linux.  OK for mainline (and 4.8 after the big patch
> series is committed)?
>
> Bye,
> Ulrich
>
> ChangeLog:
>
>         * config/rs6000/rs6000.c (fusion_gpr_load_p): Refuse optimization
>         if it would clobber the stack pointer, even temporarily.

Okay.

Thanks, David

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 208870)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -32519,6 +32519,11 @@ 
 
       if (!peep2_reg_dead_p (2, addis_reg))
 	return false;
+
+      /* If the target register being loaded is the stack pointer, we must
+         avoid loading any other value into it, even temporarily.  */
+      if (REG_P (target) && REGNO (target) == STACK_POINTER_REGNUM)
+	return false;
     }
 
   base_reg = XEXP (addr, 0);