This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] x86: Don't use get_frame_size to finalize stack frame


On Fri, Dec 14, 2018 at 3:24 PM Jeff Law <law@redhat.com> wrote:
>
> On 12/14/18 4:01 PM, H.J. Lu wrote:
> > On Thu, Dec 13, 2018 at 11:11 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >> On Thu, Dec 13, 2018 at 6:36 PM H.J. Lu <hongjiu.lu@intel.com> wrote:
> >>> get_frame_size () returns used stack slots during compilation, which
> >>> may be optimized out later.  Since ix86_find_max_used_stack_alignment
> >>> is called by ix86_finalize_stack_frame_flags to check if stack frame
> >>> is required, there is no need to call get_frame_size () which may give
> >>> inaccurate final stack frame size.
> >>>
> >>> Tested on AVX512 machine configured with
> >>>
> >>> --with-arch=native --with-cpu=native
> >>>
> >>> OK for trunk?
> >>>
> >>>
> >>> H.J.
> >>> ---
> >>> gcc/
> >>>
> >>>         PR target/88483
> >>>         * config/i386/i386.c (ix86_finalize_stack_frame_flags): Don't
> >>>         use get_frame_size ().
> >>>
> >>> gcc/testsuite/
> >>>
> >>>         PR target/88483
> >>>         * gcc.target/i386/stackalign/pr88483.c: New test.
> >> LGTM, but you know this part of the compiler better than I.
> >>
> >> Thanks,
> >> Uros.
> >>
> >>> ---
> >>>  gcc/config/i386/i386.c                          |  1 -
> >>>  .../gcc.target/i386/stackalign/pr88483.c        | 17 +++++++++++++++++
> >>>  2 files changed, 17 insertions(+), 1 deletion(-)
> >>>  create mode 100644 gcc/testsuite/gcc.target/i386/stackalign/pr88483.c
> >>>
> >>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> >>> index caa701fe242..edc8f4f092e 100644
> >>> --- a/gcc/config/i386/i386.c
> >>> +++ b/gcc/config/i386/i386.c
> >>> @@ -12876,7 +12876,6 @@ ix86_finalize_stack_frame_flags (void)
> >>>            && flag_exceptions
> >>>            && cfun->can_throw_non_call_exceptions)
> >>>        && !ix86_frame_pointer_required ()
> >>> -      && get_frame_size () == 0
> >>>        && ix86_nsaved_sseregs () == 0
> >>>        && ix86_varargs_gpr_size + ix86_varargs_fpr_size == 0)
> >>>      {
> >>> diff --git a/gcc/testsuite/gcc.target/i386/stackalign/pr88483.c b/gcc/testsuite/gcc.target/i386/stackalign/pr88483.c
> >>> new file mode 100644
> >>> index 00000000000..5aec8fd4cf6
> >>> --- /dev/null
> >>> +++ b/gcc/testsuite/gcc.target/i386/stackalign/pr88483.c
> >>> @@ -0,0 +1,17 @@
> >>> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
> >>> +/* { dg-options "-O2 -mavx2" } */
> >>> +
> >>> +struct B
> >>> +{
> >>> +  char a[12];
> >>> +  int b;
> >>> +};
> >>> +
> >>> +struct B
> >>> +f2 (void)
> >>> +{
> >>> +  struct B x = {};
> >>> +  return x;
> >>> +}
> >>> +
> >>> +/* { dg-final { scan-assembler-not "and\[lq\]?\[^\\n\]*-\[0-9\]+,\[^\\n\]*sp" } } */
> >>> --
> >>> 2.19.2
> >>>
> > My fix triggered a latent bug in ix86_find_max_used_stack_alignment.
> > Here is the fix.  OK for trunk?
> >
> > Thanks.
> >
> > -- H.J.
> >
> >
> > 0001-x86-Properly-check-stack-reference.patch
> >
> > From 83f0b37f287ed198a3b50e2be6b0f7f5c154020e Mon Sep 17 00:00:00 2001
> > From: "H.J. Lu" <hjl.tools@gmail.com>
> > Date: Fri, 14 Dec 2018 12:21:02 -0800
> > Subject: [PATCH] x86: Properly check stack reference
> >
> > A latent bug in ix86_find_max_used_stack_alignment was uncovered by the
> > fix for PR target/88483, which caused:
> >
> > FAIL: gcc.target/i386/incoming-8.c scan-assembler andl[\\t ]*\\$-16,[\\t ]*%esp
> >
> > on i386.  ix86_find_max_used_stack_alignment failed to notice stack
> > reference via non-stack/frame registers and missed stack alignment
> > requirement.  We should track all registers which may reference stack
> > by checking registers set from stack referencing registers.
> >
> > Tested on i686 and x86-64 with
> >
> > --with-arch=native --with-cpu=native
> >
> > on AVX512 machine.  Tested on i686 and x86-64 without
> >
> > --with-arch=native --with-cpu=native
> >
> > on x86-64 machine.
> >
> >       PR target/88483
> >       * config/i386/i386.c (ix86_stack_referenced_p): New function.
> >       (ix86_find_max_used_stack_alignment): Call ix86_stack_referenced_p
> >       to check if stack is referenced.
> > ---
> >  gcc/config/i386/i386.c | 43 ++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> > index 4599ca2a7d5..bf93ec3722f 100644
> > --- a/gcc/config/i386/i386.c
> > +++ b/gcc/config/i386/i386.c
> > @@ -12777,6 +12777,18 @@ output_probe_stack_range (rtx reg, rtx end)
> >    return "";
> >  }
> >
> > +/* Return true if OP references stack frame though one of registers
> > +   in STACK_REF_REGS.  */
> > +
> > +static bool
> > +ix86_stack_referenced_p (const_rtx op, rtx *stack_ref_regs)
> > +{
> > +  for (int i = 0; i < LAST_REX_INT_REG; i++)
> > +    if (stack_ref_regs[i] && reg_mentioned_p (stack_ref_regs[i], op))
> > +      return true;
> > +  return false;
> > +}
> > +
> >  /* Return true if stack frame is required.  Update STACK_ALIGNMENT
> >     to the largest alignment, in bits, of stack slot used if stack
> >     frame is required and CHECK_STACK_SLOT is true.  */
> > @@ -12801,6 +12813,12 @@ ix86_find_max_used_stack_alignment (unsigned int &stack_alignment,
> >
> >    bool require_stack_frame = false;
> >
> > +  /* Array of hard registers which reference stack frame.  */
> > +  rtx stack_ref_regs[LAST_REX_INT_REG];
> > +  memset (stack_ref_regs, 0, sizeof (stack_ref_regs));
> > +  stack_ref_regs[STACK_POINTER_REGNUM] = stack_pointer_rtx;
> > +  stack_ref_regs[FRAME_POINTER_REGNUM] = frame_pointer_rtx;
> > +
> >    FOR_EACH_BB_FN (bb, cfun)
> >      {
> >        rtx_insn *insn;
> > @@ -12811,16 +12829,33 @@ ix86_find_max_used_stack_alignment (unsigned int &stack_alignment,
> >         {
> >           require_stack_frame = true;
> >
> > +         rtx body = PATTERN (insn);
> > +         if (GET_CODE (body) == SET)
> > +           {
> > +             /* If a register is set from a stack referencing
> > +                register, it is a stack referencing register.  */
> > +             rtx dest = SET_DEST (body);
> > +             if (REG_P (dest) && !MEM_P (SET_SRC (body)))
> > +               {
> > +                 int regno = REGNO (dest);
> > +                 gcc_assert (regno < FIRST_PSEUDO_REGISTER);
> > +                 if (GENERAL_REGNO_P (regno))
> > +                   {
> > +                     add_to_hard_reg_set (&set_up_by_prologue, Pmode,
> > +                                          regno);
> > +                     stack_ref_regs[regno] = dest;
> > +                   }
> > +               }
> > +           }
> > +
> >           if (check_stack_slot)
> >             {
> >               /* Find the maximum stack alignment.  */
> >               subrtx_iterator::array_type array;
> >               FOR_EACH_SUBRTX (iter, array, PATTERN (insn), ALL)
> >                 if (MEM_P (*iter)
> > -                   && (reg_mentioned_p (stack_pointer_rtx,
> > -                                        *iter)
> > -                       || reg_mentioned_p (frame_pointer_rtx,
> > -                                           *iter)))
> > +                   && ix86_stack_referenced_p (*iter,
> > +                                               stack_ref_regs))
> >                   {
> >                     unsigned int alignment = MEM_ALIGN (*iter);
> >                     if (alignment > stack_alignment)
> This just does a linear scan of the blocks and insns within the block
> building up STACK_REF_REGS as we go.
>
> The problem is there's no guarantee that we're visiting the blocks in
> execution order.  So we might see an insn that indirectly references the
> stack reg *before* we see the insn which had the copy from the stack
> pointer.
>
> Or am I missing something?
>

You are right.  We must be conservative in this case   Here is the
updated patch.  If there may be indirect stack references, stop and
restore the original stack alignment.

I am testing it now.

-- 
H.J.
From dc06e6e794c6d3949e1cd5d1e725a1d06cba27d3 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 14 Dec 2018 12:21:02 -0800
Subject: [PATCH] x86: Properly check stack alignment

A latent bug in ix86_find_max_used_stack_alignment was uncovered by the
fix for PR target/88483, which caused:

FAIL: gcc.target/i386/incoming-8.c scan-assembler andl[\\t ]*\\$-16,[\\t ]*%esp

on i386.  ix86_find_max_used_stack_alignment failed to notice stack
reference via non-stack/frame registers and missed stack alignment
requirement.  There's no guarantee that we're visiting the blocks in
execution order.  If there may be indirect stack references, stop and
restore the original stack alignment.

	PR target/88483
	* config/i386/i386.c (ix86_find_max_used_stack_alignment):
	If there may be indirect stack references, stop and restore the
	original stack alignment.
---
 gcc/config/i386/i386.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 4599ca2a7d5..22179d21060 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -12795,6 +12795,9 @@ ix86_find_max_used_stack_alignment (unsigned int &stack_alignment,
   add_to_hard_reg_set (&set_up_by_prologue, Pmode,
 		       HARD_FRAME_POINTER_REGNUM);
 
+  /* Save the original stack alignment.  */
+  unsigned int original_stack_alignment = stack_alignment;
+
   /* The preferred stack alignment is the minimum stack alignment.  */
   if (stack_alignment > crtl->preferred_stack_boundary)
     stack_alignment = crtl->preferred_stack_boundary;
@@ -12811,6 +12814,26 @@ ix86_find_max_used_stack_alignment (unsigned int &stack_alignment,
 	  {
 	    require_stack_frame = true;
 
+	    rtx body = PATTERN (insn);
+	    if (GET_CODE (body) == SET)
+	      {
+		/* If a register is set from a stack referencing
+		   register, it may be used to reference stack
+		   indirectly.  */
+		rtx dest = SET_DEST (body);
+		if (REG_P (dest)
+		    && GENERAL_REG_P (dest)
+		    && !MEM_P (SET_SRC (body)))
+		  {
+		    /* There's no guarantee that we're visiting the
+		       blocks in execution order.  If there may be
+		       indirect stack references, stop and restore
+		       the original stack alignment.  */
+		    stack_alignment = original_stack_alignment;
+		    return require_stack_frame;
+		  }
+	      }
+
 	    if (check_stack_slot)
 	      {
 		/* Find the maximum stack alignment.  */
-- 
2.19.2


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]