diff mbox

Fix some x86 peepholes vs. the red-zone

Message ID 570CE2B8.7070303@redhat.com
State New
Headers show

Commit Message

Bernd Schmidt April 12, 2016, 11:57 a.m. UTC 
With some changes I was working on, I produced a situation where one of 
the x86 peepholes made an incorrect transformation. In the prologue 
expansion code, we have

/* When using red zone we may start register saving before allocating
    the stack frame saving one cycle of the prologue.  However, avoid
    doing this if we have to probe the stack; at least on x86_64 the
    stack probe can turn into a call that clobbers a red zone location.

So, we can in some situations produce something like:

mov %ebx, -8(%rsp)
mov %edi, -16(%rsp)
subl $16, %rsp

which is fine if using a redzone. The problem is that there are 
peepholes which convert sp subtractions into pushes, clobbering the 
saved registers.

I've made these peepholes conditional on !x86_using_red_zone. 
Bootstrapped and tested on x86_64-linux, ok (now or later)?


Bernd

Comments

Jeff Law April 14, 2016, 5:31 p.m. UTC | #1 
On 04/12/2016 05:57 AM, Bernd Schmidt wrote:
> With some changes I was working on, I produced a situation where one of
> the x86 peepholes made an incorrect transformation. In the prologue
> expansion code, we have
>
> /* When using red zone we may start register saving before allocating
>     the stack frame saving one cycle of the prologue.  However, avoid
>     doing this if we have to probe the stack; at least on x86_64 the
>     stack probe can turn into a call that clobbers a red zone location.
>
> So, we can in some situations produce something like:
>
> mov %ebx, -8(%rsp)
> mov %edi, -16(%rsp)
> subl $16, %rsp
>
> which is fine if using a redzone. The problem is that there are
> peepholes which convert sp subtractions into pushes, clobbering the
> saved registers.
>
> I've made these peepholes conditional on !x86_using_red_zone.
> Bootstrapped and tested on x86_64-linux, ok (now or later)?
OK for stage1.  Uros's call whether or not to OK for the trunk right now.

jeff
Bernd Schmidt April 15, 2016, 1:36 p.m. UTC | #2 
On 04/15/2016 10:32 AM, Uros Bizjak wrote:
> This fixes possible wrong code with a tricky failure mode, so OK now.

Thanks.

[...]

> While changing this part, it can be rewritten using dg-additional options, e.g.:
>
> /* { dg-options "-Os -fomit-frame-pointer -fasynchronous-unwind-tables
> -mno-red-zone" } */
> /* { dg-additional-options "-mpreferred-stack-boundary=3" { target ia32 } } */

Since Jakub mentioned branching today, and I wasn't sure I'd have time 
for a retest, I've committed it as-is.


Bernd
diff mbox

Patch

	* config/i386/i386-protos.h (ix86_using_red_zone): Declare.
	* config/i386/i386.c (ix86_using_red_zone): No longer static.
	* config/i386/i386.md (stack decrement to push peepholes): Guard
	with !x86_using_red_zone ().

testsuite/
	* gcc.target/i386/pr46470.c: Add -mno-red-zone to dg-optios for
	x86_64.

Index: gcc/config/i386/i386-protos.h
===================================================================
--- gcc/config/i386/i386-protos.h	(revision 234831)
+++ gcc/config/i386/i386-protos.h	(working copy)
@@ -44,6 +44,8 @@  extern bool ix86_use_pseudo_pic_reg (voi
 
 extern void ix86_reset_previous_fndecl (void);
 
+extern bool ix86_using_red_zone (void);
+
 #ifdef RTX_CODE
 extern int standard_80387_constant_p (rtx);
 extern const char *standard_80387_constant_opcode (rtx);
Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c	(revision 234831)
+++ gcc/config/i386/i386.c	(working copy)
@@ -3709,7 +3709,7 @@  make_pass_stv (gcc::context *ctxt)
 
 /* Return true if a red-zone is in use.  */
 
-static inline bool
+bool
 ix86_using_red_zone (void)
 {
   return TARGET_RED_ZONE && !TARGET_64BIT_MS_ABI;
Index: gcc/config/i386/i386.md
===================================================================
--- gcc/config/i386/i386.md	(revision 234831)
+++ gcc/config/i386/i386.md	(working copy)
@@ -18240,7 +18240,8 @@  (define_peephole2
 	      (clobber (reg:CC FLAGS_REG))
 	      (clobber (mem:BLK (scratch)))])]
   "(TARGET_SINGLE_PUSH || optimize_insn_for_size_p ())
-   && INTVAL (operands[0]) == -GET_MODE_SIZE (word_mode)"
+   && INTVAL (operands[0]) == -GET_MODE_SIZE (word_mode)
+   && !ix86_using_red_zone ()"
   [(clobber (match_dup 1))
    (parallel [(set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))
 	      (clobber (mem:BLK (scratch)))])])
@@ -18253,7 +18254,8 @@  (define_peephole2
 	      (clobber (reg:CC FLAGS_REG))
 	      (clobber (mem:BLK (scratch)))])]
   "(TARGET_DOUBLE_PUSH || optimize_insn_for_size_p ())
-   && INTVAL (operands[0]) == -2*GET_MODE_SIZE (word_mode)"
+   && INTVAL (operands[0]) == -2*GET_MODE_SIZE (word_mode)
+   && !ix86_using_red_zone ()"
   [(clobber (match_dup 1))
    (set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))
    (parallel [(set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))
@@ -18267,7 +18269,8 @@  (define_peephole2
 			   (match_operand:P 0 "const_int_operand")))
 	      (clobber (reg:CC FLAGS_REG))])]
   "(TARGET_SINGLE_PUSH || optimize_insn_for_size_p ())
-   && INTVAL (operands[0]) == -GET_MODE_SIZE (word_mode)"
+   && INTVAL (operands[0]) == -GET_MODE_SIZE (word_mode)
+   && !ix86_using_red_zone ()"
   [(clobber (match_dup 1))
    (set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))])
 
@@ -18278,7 +18281,8 @@  (define_peephole2
 			   (match_operand:P 0 "const_int_operand")))
 	      (clobber (reg:CC FLAGS_REG))])]
   "(TARGET_DOUBLE_PUSH || optimize_insn_for_size_p ())
-   && INTVAL (operands[0]) == -2*GET_MODE_SIZE (word_mode)"
+   && INTVAL (operands[0]) == -2*GET_MODE_SIZE (word_mode)
+   && !ix86_using_red_zone ()"
   [(clobber (match_dup 1))
    (set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))
    (set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))])
Index: gcc/testsuite/gcc.target/i386/pr46470.c
===================================================================
--- gcc/testsuite/gcc.target/i386/pr46470.c	(revision 234831)
+++ gcc/testsuite/gcc.target/i386/pr46470.c	(working copy)
@@ -4,7 +4,7 @@ 
 /* These options are selected to ensure 1 word needs to be allocated
    on the stack to maintain alignment for the call.  This should be
    transformed to push+pop.  We also want to force unwind info updates.  */
-/* { dg-options "-Os -fomit-frame-pointer -fasynchronous-unwind-tables" } */
+/* { dg-options "-Os -fomit-frame-pointer -fasynchronous-unwind-tables -mno-red-zone" } */
 /* { dg-options "-Os -fomit-frame-pointer -mpreferred-stack-boundary=3 -fasynchronous-unwind-tables" { target ia32 } } */
 /* ms_abi has reserved stack-region.  */
 /* { dg-skip-if "" { x86_64-*-mingw* } { "*" } { "" } } */