Re: [RFA][PATCH] Improve initial probe for noreturn functions for x86 target

[Jeff Law <law at redhat dot com>]

On 11/03/2017 11:38 AM, Uros Bizjak wrote:
> On Fri, Nov 3, 2017 at 6:13 PM, Jeff Law <law@redhat.com> wrote:
> > On 11/03/2017 04:46 AM, Uros Bizjak wrote:
> > > On Fri, Nov 3, 2017 at 11:14 AM, Richard Biener
> > > <richard.guenther@gmail.com> wrote:
> > > > On Fri, Nov 3, 2017 at 9:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > > > > >              * config/i386/i386.c (ix86_emit_restore_reg_using_pop):
> > > > > > Prototype.
> > > > > >              (ix86_adjust_stack_and_probe_stack_clash): Use a push/pop
> > > > > > sequence
> > > > > >              to probe at the start of a noreturn function.
> > > > > >
> > > > > >              * gcc.target/i386/stack-check-12.c: New test
> > > > >
> > > > >
> > > > > -      emit_stack_probe (plus_constant (Pmode, stack_pointer_rtx,
> > > > > -       -GET_MODE_SIZE (word_mode)));
> > > > > +      rtx_insn *insn = emit_insn (gen_push (gen_rtx_REG (word_mode,
> > > > > 0)));
> > > > >
> > > > > Please use AX_REG instead of 0.
> > > > >
> > > > > +      RTX_FRAME_RELATED_P (insn) = 1;
> > > > > +      ix86_emit_restore_reg_using_pop (gen_rtx_REG (word_mode, 0));
> > > > >
> > > > > Also here.
> > > > >
> > > > >          emit_insn (gen_blockage ());
> > > > >
> > > > > BTW: Could we use an unused register here, if available? %eax is used
> > > > > to pass first argument in regparm functions on 32bit targets.
> > > >
> > > >
> > > > Can you push %[er]sp?  What about partial reg stalls when using other
> > > > registers (if the last set was a movb to it)?  I guess [er]sp is safe
> > > > here
> > > > as was [re]ax due to the ABI?
> > >
> > >
> > > That would work, too. I believe, that this won't trigger stack engine
> > > [1], but since the operation is a bit unusual, let's ask HJ to be
> > > sure.
> > >
> > > [1] https://en.wikipedia.org/wiki/Stack_register#Stack_engine
> >
> > How about %esi in 32 bit mode and %rax in 64 bit mode?  I think that avoids
> > hitting the parameter passing registers.
>
> That is a good choice. IMO, It warrants a small comment, in the
> source, why this choice.
Here's a patch implementing that approach.

Bootstrapped and regression tested on x86_64.  Also spot tested the new 
test on x86.

OK for the trunk now?

Jeff

            * config/i386/i386.c (ix86_emit_restore_reg_using_pop): Prototype.
            (ix86_adjust_stack_and_probe_stack_clash): Use a push/pop sequence
            to probe at the start of a noreturn function.
    
            * gcc.target/i386/stack-check-12.c: New test


diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 25b28a1..1b83755 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -101,6 +101,8 @@ static void ix86_print_operand_address_as (FILE *, rtx, addr_space_t, bool);
 static bool ix86_save_reg (unsigned int, bool, bool);
 static bool ix86_function_naked (const_tree);
 static bool ix86_notrack_prefixed_insn_p (rtx);
+static void ix86_emit_restore_reg_using_pop (rtx);
+
 
 #ifndef CHECK_STACK_LIMIT
 #define CHECK_STACK_LIMIT (-1)
@@ -12124,8 +12126,14 @@ ix86_adjust_stack_and_probe_stack_clash (const HOST_WIDE_INT size)
      we just probe when we cross PROBE_INTERVAL.  */
   if (TREE_THIS_VOLATILE (cfun->decl))
     {
-      emit_stack_probe (plus_constant (Pmode, stack_pointer_rtx,
-				       -GET_MODE_SIZE (word_mode)));
+      /* We can safely use any register here since we're just going to push
+	 its value and immediately pop it back.  But we do try and avoid
+	 argument passing registers so as not to introduce dependencies in
+	 the pipeline.  For 32 bit we use %esi and for 64 bit we use %rax.  */
+      rtx dummy_reg = gen_rtx_REG (word_mode, TARGET_64BIT ? AX_REG : SI_REG);
+      rtx_insn *insn = emit_insn (gen_push (dummy_reg));
+      RTX_FRAME_RELATED_P (insn) = 1;
+      ix86_emit_restore_reg_using_pop (dummy_reg);
       emit_insn (gen_blockage ());
     }
 
diff --git a/gcc/testsuite/gcc.target/i386/stack-check-12.c b/gcc/testsuite/gcc.target/i386/stack-check-12.c
new file mode 100644
index 0000000..cb69bb0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/stack-check-12.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fstack-clash-protection -mtune=generic" } */
+/* { dg-require-effective-target supports_stack_clash_protection } */
+
+__attribute__ ((noreturn)) void exit (int);
+
+__attribute__ ((noreturn)) void
+f (void)
+{
+  asm volatile ("nop" ::: "edi");
+  exit (1);
+}
+
+/* { dg-final { scan-assembler-not "or\[ql\]" } } */
+/* { dg-final { scan-assembler "pushl	%esi" { target ia32 } } } */
+/* { dg-final { scan-assembler "popl	%esi" { target ia32 } } }*/
+/* { dg-final { scan-assembler "pushq	%rax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "popq	%rax" { target { ! ia32 } } } }*/
+