| Message ID | CAEwic4YRnyDhML9GYvM1+OyrTdX+-F4HagKJWyfjDnNhLkM4wQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
ping
> 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));
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
> 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
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.
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
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
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));
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