Patchwork [i386] : Fix PR 56807

login
register
mail settings
Submitter Kai Tietz
Date Dec. 6, 2013, 5:06 p.m.
Message ID <CAEwic4YRnyDhML9GYvM1+OyrTdX+-F4HagKJWyfjDnNhLkM4wQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/298104/
State New
Headers show

Comments

Kai Tietz - Dec. 6, 2013, 5:06 p.m.
Hi,


ChangeLog

2013-12-06  Kai Tietz  <ktietz@redhat.com>

    PR target/56807
    * config/i386/i386.c (ix86_expand_prologue):

Tested for i686-w64-mingw32, x86_64-unknown-linux-gnu.  Ok for apply?

Regards,
Kai
H.J. Lu - Dec. 6, 2013, 5:10 p.m.
On Fri, Dec 6, 2013 at 9:06 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hi,
>
>
> ChangeLog
>
> 2013-12-06  Kai Tietz  <ktietz@redhat.com>
>
>     PR target/56807
>     * config/i386/i386.c (ix86_expand_prologue):
>

Incomplete ChangeLog entry.
Kai Tietz - Dec. 6, 2013, 5:12 p.m.
Upps ... here is the missing Changlog

ChangeLog

2013-12-06  Kai Tietz  <ktietz@redhat.com>

    PR target/56807
    * config/i386/i386.c (ix86_expand_prologue): Address saved
    registers stack-relative, not via frame-pointer.
Kai Tietz - Dec. 9, 2013, 11:24 a.m.
ping
Jan Hubicka - Dec. 10, 2013, 11:48 a.m.
> Hi,
> 
> 
> ChangeLog
> 
> 2013-12-06  Kai Tietz  <ktietz@redhat.com>
> 
>     PR target/56807
>     * config/i386/i386.c (ix86_expand_prologue):
> 
> Tested for i686-w64-mingw32, x86_64-unknown-linux-gnu.  Ok for apply?

So the code in question does spilling relative to stack pointer before function
call and relative to base pointer after the function call and this leads to mismatch
when stack alignment is in the way?

The chagne seems OK if that is the case.

Honza
> 
> Regards,
> Kai
> 
> Index: config/i386/i386.c
> ===================================================================
> --- config/i386/i386.c    (Revision 205719)
> +++ config/i386/i386.c    (Arbeitskopie)
> @@ -10934,18 +10937,21 @@ ix86_expand_prologue (void)
>      }
>        m->fs.sp_offset += allocate;
> 
> +      /* Use stack_pointer_rtx for relative addressing so that code
> +     works for realigned stack, too.  */
>        if (r10_live && eax_live)
>          {
> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>        emit_move_insn (gen_rtx_REG (word_mode, R10_REG),
>                gen_frame_mem (word_mode, t));
> -      t = choose_baseaddr (m->fs.sp_offset - allocate - UNITS_PER_WORD);
> +      t = plus_constant (Pmode, stack_pointer_rtx,
> +                 allocate - UNITS_PER_WORD);
>        emit_move_insn (gen_rtx_REG (word_mode, AX_REG),
>                gen_frame_mem (word_mode, t));
>      }
>        else if (eax_live || r10_live)
>      {
> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>        emit_move_insn (gen_rtx_REG (word_mode,
>                         (eax_live ? AX_REG : R10_REG)),
>                gen_frame_mem (word_mode, t));
Jakub Jelinek - Dec. 10, 2013, 11:56 a.m.
On Tue, Dec 10, 2013 at 12:48:20PM +0100, Jan Hubicka wrote:
> > Hi,
> > 
> > 
> > ChangeLog
> > 
> > 2013-12-06  Kai Tietz  <ktietz@redhat.com>
> > 
> >     PR target/56807
> >     * config/i386/i386.c (ix86_expand_prologue):
> > 
> > Tested for i686-w64-mingw32, x86_64-unknown-linux-gnu.  Ok for apply?
> 
> So the code in question does spilling relative to stack pointer before function
> call and relative to base pointer after the function call and this leads to mismatch
> when stack alignment is in the way?
> 
> The chagne seems OK if that is the case.

The ChangeLog entry is not ok though.

	Jakub
Jan Hubicka - Dec. 10, 2013, 12:01 p.m.
> On Tue, Dec 10, 2013 at 12:48:20PM +0100, Jan Hubicka wrote:
> > > Hi,
> > > 
> > > 
> > > ChangeLog
> > > 
> > > 2013-12-06  Kai Tietz  <ktietz@redhat.com>
> > > 
> > >     PR target/56807
> > >     * config/i386/i386.c (ix86_expand_prologue):
> > > 
> > > Tested for i686-w64-mingw32, x86_64-unknown-linux-gnu.  Ok for apply?
> > 
> > So the code in question does spilling relative to stack pointer before function
> > call and relative to base pointer after the function call and this leads to mismatch
> > when stack alignment is in the way?
> > 
> > The chagne seems OK if that is the case.
> 
> The ChangeLog entry is not ok though.

Kai sent updated one in reply to Jeff's email I beleive.

honza
> 
> 	Jakub
Kai Tietz - Dec. 10, 2013, 1:05 p.m.
Yes, I sent update to HJ's comment.

2013/12/6 Kai Tietz <ktietz70@googlemail.com>:
> Upps ... here is the missing Changlog
>
> ChangeLog
>
> 2013-12-06  Kai Tietz  <ktietz@redhat.com>
>
>     PR target/56807
>     * config/i386/i386.c (ix86_expand_prologue): Address saved
>     registers stack-relative, not via frame-pointer.
Jakub Jelinek - Dec. 16, 2013, 6:13 p.m.
On Fri, Dec 06, 2013 at 06:06:14PM +0100, Kai Tietz wrote:
> --- config/i386/i386.c    (Revision 205719)
> +++ config/i386/i386.c    (Arbeitskopie)
> @@ -10934,18 +10937,21 @@ ix86_expand_prologue (void)
>      }
>        m->fs.sp_offset += allocate;
> 
> +      /* Use stack_pointer_rtx for relative addressing so that code
> +     works for realigned stack, too.  */
>        if (r10_live && eax_live)
>          {
> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>        emit_move_insn (gen_rtx_REG (word_mode, R10_REG),
>                gen_frame_mem (word_mode, t));
> -      t = choose_baseaddr (m->fs.sp_offset - allocate - UNITS_PER_WORD);
> +      t = plus_constant (Pmode, stack_pointer_rtx,
> +                 allocate - UNITS_PER_WORD);

Somebody just asked on IRC whether this shouldn't have been
allocate + UNITS_PER_WORD.

Dunno when would be eax_live true on x86_64 though (except for uninitialized
var uses).

>        emit_move_insn (gen_rtx_REG (word_mode, AX_REG),
>                gen_frame_mem (word_mode, t));
>      }
>        else if (eax_live || r10_live)
>      {
> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>        emit_move_insn (gen_rtx_REG (word_mode,
>                         (eax_live ? AX_REG : R10_REG)),
>                gen_frame_mem (word_mode, t));

	Jakub
Kai Tietz - Dec. 16, 2013, 7:16 p.m.
2013/12/16 Jakub Jelinek <jakub@redhat.com>:
> On Fri, Dec 06, 2013 at 06:06:14PM +0100, Kai Tietz wrote:
>> --- config/i386/i386.c    (Revision 205719)
>> +++ config/i386/i386.c    (Arbeitskopie)
>> @@ -10934,18 +10937,21 @@ ix86_expand_prologue (void)
>>      }
>>        m->fs.sp_offset += allocate;
>>
>> +      /* Use stack_pointer_rtx for relative addressing so that code
>> +     works for realigned stack, too.  */
>>        if (r10_live && eax_live)
>>          {
>> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
>> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>>        emit_move_insn (gen_rtx_REG (word_mode, R10_REG),
>>                gen_frame_mem (word_mode, t));
>> -      t = choose_baseaddr (m->fs.sp_offset - allocate - UNITS_PER_WORD);
>> +      t = plus_constant (Pmode, stack_pointer_rtx,
>> +                 allocate - UNITS_PER_WORD);
>
> Somebody just asked on IRC whether this shouldn't have been
> allocate + UNITS_PER_WORD.

Well, I had over weekend same discussion on irc.  AFAIR it was BugMaster ...
and yes, in the case (for x86_64 possible only) that r10_live and
eax_live there seems to be a bug.

The addressing of save-region for r10, which is saved after rax, is
correct.  The restore-address of rax is wrong.  It should be
         t = plus_constant (Pmode, stack_pointer_rtx,
                            allocate + UNITS_PER_WORD);
instead.

> Dunno when would be eax_live true on x86_64 though (except for uninitialized
> var uses).
>

Kai

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c    (Revision 205719)
+++ config/i386/i386.c    (Arbeitskopie)
@@ -10934,18 +10937,21 @@  ix86_expand_prologue (void)
     }
       m->fs.sp_offset += allocate;

+      /* Use stack_pointer_rtx for relative addressing so that code
+     works for realigned stack, too.  */
       if (r10_live && eax_live)
         {
-      t = choose_baseaddr (m->fs.sp_offset - allocate);
+      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
       emit_move_insn (gen_rtx_REG (word_mode, R10_REG),
               gen_frame_mem (word_mode, t));
-      t = choose_baseaddr (m->fs.sp_offset - allocate - UNITS_PER_WORD);
+      t = plus_constant (Pmode, stack_pointer_rtx,
+                 allocate - UNITS_PER_WORD);
       emit_move_insn (gen_rtx_REG (word_mode, AX_REG),
               gen_frame_mem (word_mode, t));
     }
       else if (eax_live || r10_live)
     {
-      t = choose_baseaddr (m->fs.sp_offset - allocate);
+      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
       emit_move_insn (gen_rtx_REG (word_mode,
                        (eax_live ? AX_REG : R10_REG)),
               gen_frame_mem (word_mode, t));