Re: [PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true
Hi! On Mon, Mar 30, 2020 at 12:18:59PM +0800, luoxhu wrote: > >>>If df_regs_ever_live_p(31) is false there is no hard frame pointer > >>>anywhere in the program. How can it be used then? > >> > >>There is a piece of code emit move instruction to r31 even > >>df_regs_ever_live_p(31) is false > >>in pro_and_epilogue. > > > >Can you point out where (in rs6000-logue.c ot similar)? We should fix > >*that*. > >frame_pointer_needed is often true when the backend can figure out we do > >not actually need it. > >(so just that "if" clause changes), it'll all be fine. Could you test > >that please? > > Tried with below patch, it still fails at same place, I guess this is not > enough. > The instruction in 100dc034 change r31 from 0x105fdd70 to 0x7fffbbf0 > and didn't > restore it before return. So where was *that* insn generated, then? Segher
Re: [PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true
On 3/30/20 12:18 AM, luoxhu via Gcc-patches wrote:
On 2020/3/28 00:04, Segher Boessenkool wrote:
Hi!
On Fri, Mar 27, 2020 at 09:34:00AM +0800, luoxhu wrote:
On 2020/3/27 07:59, Segher Boessenkool wrote:
On Wed, Mar 25, 2020 at 11:15:22PM -0500, luo...@linux.ibm.com wrote:
frame_pointer_needed is set to true in reload pass
setup_can_eliminate,
but regs_ever_live[31] is false, so pro_and_epilogue doesn't
save/restore
r31 even it is used actually, causing CPU2006 465.tonto segment
fault of
loading from invalid addresses.
If df_regs_ever_live_p(31) is false there is no hard frame pointer
anywhere in the program. How can it be used then?
There is a piece of code emit move instruction to r31 even
df_regs_ever_live_p(31) is false
in pro_and_epilogue.
Can you point out where (in rs6000-logue.c ot similar)? We should fix
*that*.
As frame_point_needed is true and frame_pointer_needed is widely
used in this function, so I propose to save r31 in save_reg_p
instead of check
(frame_pointer_needed && df_regs_ever_live_p(31), I haven't verify
whether this works yet).
Is this reasonable? Thanks.
frame_pointer_needed is often true when the backend can figure out we do
not actually need it.
rs6000-logue.c
void
rs6000_emit_prologue (void)
{
...
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26840) /*
Set frame pointer, if needed. */
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26841) if
(frame_pointer_needed)
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26842) {
0d6c02bf24e4 (jakub 2005-06-30 14:26:32 + 26843)
insn = emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 +
26844) sp_reg_rtx);
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26845)
RTX_FRAME_RELATED_P (insn) = 1;
6b02f2a5c61e (meissner 1995-11-30 20:02:16 + 26846) }
d1bd513ed578 (kenner 1992-02-09 19:26:21 + 26847)
...
}
Ah, so this you mean. I see. It looks like if you change this to
if (frame_pointer_needed && df_regs_ever_live_p
(HARD_FRAME_POINTER_REGNUM))
{
insn = emit_move_insn (gen_rtx_REG (Pmode,
HARD_FRAME_POINTER_REGNUM),
sp_reg_rtx);
RTX_FRAME_RELATED_P (insn) = 1;
}
(so just that "if" clause changes), it'll all be fine. Could you test
that please?
Tried with below patch, it still fails at same place, I guess this is
not enough.
The instruction in 100dc034 change r31 from 0x105fdd70 to
0x7fffbbf0 and didn't
restore it before return.
100dc020 <__atomvec_module_MOD_atom_index_from_pos>:
100dc020: 51 10 40 3c lis r2,4177
100dc024: 00 7f 42 38 addi r2,r2,32512
100dc028: 28 00 e3 e8 ld r7,40(r3)
100dc02c: e1 ff 21 f8 stdu r1,-32(r1)
100dc030: 00 00 27 2c cmpdi r7,0
100dc034: 78 0b 3f 7c mr r31,r1
100dc038: 08 00 82 40 bne 100dc040
<__atomvec_module_MOD_atom_index_from_pos+0x20>
100dc03c: 01 00 e0 38 li r7,1
100dc040: 38 00 43 e9 ld r10,56(r3)
100dc044: 30 00 23 e9 ld r9,48(r3)
100dc048: 00 00 03 e9 ld r8,0(r3)
Diff:
diff --git a/gcc/config/rs6000/rs6000-logue.c
b/gcc/config/rs6000/rs6000-logue.c
index 4cbf228eb79..28fda1866d8 100644
--- a/gcc/config/rs6000/rs6000-logue.c
+++ b/gcc/config/rs6000/rs6000-logue.c
@@ -3658,7 +3658,7 @@ rs6000_emit_prologue (void)
}
/* Set frame pointer, if needed. */
- if (frame_pointer_needed)
+ if (frame_pointer_needed && df_regs_ever_live_p
(HARD_FRAME_POINTER_REGNUM))
{
insn = emit_move_insn (gen_rtx_REG (Pmode,
HARD_FRAME_POINTER_REGNUM),
sp_reg_rtx);
@@ -4534,7 +4534,8 @@ rs6000_emit_epilogue (enum epilogue_type
epilogue_type)
}
/* If we have a frame pointer, we can restore the old stack pointer
from it. */
- else if (frame_pointer_needed)
+ else if (frame_pointer_needed
+ && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM))
{
frame_reg_rtx = sp_reg_rtx;
if (DEFAULT_ABI == ABI_V4)
@@ -4873,7 +4874,8 @@ rs6000_emit_epilogue (enum epilogue_type
epilogue_type)
a REG_CFA_DEF_CFA note, but that's OK; A duplicate is
discarded by dwarf2cfi.c/dwarf2out.c, and in any case would
be harmless if emitted. */
- if (frame_pointer_needed)
+ if (frame_pointer_needed
+ && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM))
{
insn = get_last_insn ();
add_reg_note (insn, REG_CFA_DEF_CFA,
Program received signal SIGSEGV: Segmentation fault - invalid memory
reference.
Backtrace for this error:
#0 0x200a2243 in ???
#1 0x200a0abb in ???
#2 0x200604d7 in ???
#3 0x1027418c in __mol_module_MOD_make_image_of_shell
at
/home/luoxhu/workspace/cpu2006/benchspec/CPU2006/465.tonto/build/build_peak_none./mol.fppized.f90:9376
Re: [PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true
On 2020/3/28 00:04, Segher Boessenkool wrote:
Hi!
On Fri, Mar 27, 2020 at 09:34:00AM +0800, luoxhu wrote:
On 2020/3/27 07:59, Segher Boessenkool wrote:
On Wed, Mar 25, 2020 at 11:15:22PM -0500, luo...@linux.ibm.com wrote:
frame_pointer_needed is set to true in reload pass setup_can_eliminate,
but regs_ever_live[31] is false, so pro_and_epilogue doesn't save/restore
r31 even it is used actually, causing CPU2006 465.tonto segment fault of
loading from invalid addresses.
If df_regs_ever_live_p(31) is false there is no hard frame pointer
anywhere in the program. How can it be used then?
There is a piece of code emit move instruction to r31 even
df_regs_ever_live_p(31) is false
in pro_and_epilogue.
Can you point out where (in rs6000-logue.c ot similar)? We should fix
*that*.
As frame_point_needed is true and frame_pointer_needed is widely
used in this function, so I propose to save r31 in save_reg_p instead of check
(frame_pointer_needed && df_regs_ever_live_p(31), I haven't verify whether this
works yet).
Is this reasonable? Thanks.
frame_pointer_needed is often true when the backend can figure out we do
not actually need it.
rs6000-logue.c
void
rs6000_emit_prologue (void)
{
...
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26840) /* Set frame
pointer, if needed. */
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26841) if
(frame_pointer_needed)
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26842) {
0d6c02bf24e4 (jakub 2005-06-30 14:26:32 + 26843) insn =
emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26844)
sp_reg_rtx);
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26845)
RTX_FRAME_RELATED_P (insn) = 1;
6b02f2a5c61e (meissner 1995-11-30 20:02:16 + 26846) }
d1bd513ed578 (kenner 1992-02-09 19:26:21 + 26847)
...
}
Ah, so this you mean. I see. It looks like if you change this to
if (frame_pointer_needed && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM))
{
insn = emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
sp_reg_rtx);
RTX_FRAME_RELATED_P (insn) = 1;
}
(so just that "if" clause changes), it'll all be fine. Could you test
that please?
Tried with below patch, it still fails at same place, I guess this is not
enough.
The instruction in 100dc034 change r31 from 0x105fdd70 to 0x7fffbbf0 and
didn't
restore it before return.
100dc020 <__atomvec_module_MOD_atom_index_from_pos>:
100dc020: 51 10 40 3c lis r2,4177
100dc024: 00 7f 42 38 addir2,r2,32512
100dc028: 28 00 e3 e8 ld r7,40(r3)
100dc02c: e1 ff 21 f8 stdur1,-32(r1)
100dc030: 00 00 27 2c cmpdi r7,0
100dc034: 78 0b 3f 7c mr r31,r1
100dc038: 08 00 82 40 bne 100dc040
<__atomvec_module_MOD_atom_index_from_pos+0x20>
100dc03c: 01 00 e0 38 li r7,1
100dc040: 38 00 43 e9 ld r10,56(r3)
100dc044: 30 00 23 e9 ld r9,48(r3)
100dc048: 00 00 03 e9 ld r8,0(r3)
Diff:
diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
index 4cbf228eb79..28fda1866d8 100644
--- a/gcc/config/rs6000/rs6000-logue.c
+++ b/gcc/config/rs6000/rs6000-logue.c
@@ -3658,7 +3658,7 @@ rs6000_emit_prologue (void)
}
/* Set frame pointer, if needed. */
- if (frame_pointer_needed)
+ if (frame_pointer_needed && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM))
{
insn = emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
sp_reg_rtx);
@@ -4534,7 +4534,8 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type)
}
/* If we have a frame pointer, we can restore the old stack pointer
from it. */
- else if (frame_pointer_needed)
+ else if (frame_pointer_needed
+ && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM))
{
frame_reg_rtx = sp_reg_rtx;
if (DEFAULT_ABI == ABI_V4)
@@ -4873,7 +4874,8 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type)
a REG_CFA_DEF_CFA note, but that's OK; A duplicate is
discarded by dwarf2cfi.c/dwarf2out.c, and in any case would
be harmless if emitted. */
- if (frame_pointer_needed)
+ if (frame_pointer_needed
+ && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM))
{
insn = get_last_insn ();
add_reg_note (insn, REG_CFA_DEF_CFA,
Program received signal SIGSEGV: Segmentation fault - invalid memory reference.
Backtrace for this error:
#0 0x200a2243 in ???
#1 0x200a0abb in ???
#2 0x200604d7 in ???
#3 0x1027418c in __mol_module_MOD_make_image_of_shell
at
/home/luoxhu/workspace/cpu2006/benchspec/CPU2006/465.tonto/build/build_peak_none./mol.fppized.f90:9376
#4 0x10281adf in __mol_module_MOD_symmetrise_r
at
Re: [PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true
Hi!
On Fri, Mar 27, 2020 at 09:34:00AM +0800, luoxhu wrote:
> On 2020/3/27 07:59, Segher Boessenkool wrote:
> > On Wed, Mar 25, 2020 at 11:15:22PM -0500, luo...@linux.ibm.com wrote:
> >> frame_pointer_needed is set to true in reload pass setup_can_eliminate,
> >> but regs_ever_live[31] is false, so pro_and_epilogue doesn't save/restore
> >> r31 even it is used actually, causing CPU2006 465.tonto segment fault of
> >> loading from invalid addresses.
> >
> > If df_regs_ever_live_p(31) is false there is no hard frame pointer
> > anywhere in the program. How can it be used then?
>
> There is a piece of code emit move instruction to r31 even
> df_regs_ever_live_p(31) is false
> in pro_and_epilogue.
Can you point out where (in rs6000-logue.c ot similar)? We should fix
*that*.
> As frame_point_needed is true and frame_pointer_needed is widely
> used in this function, so I propose to save r31 in save_reg_p instead of check
> (frame_pointer_needed && df_regs_ever_live_p(31), I haven't verify whether
> this works yet).
> Is this reasonable? Thanks.
frame_pointer_needed is often true when the backend can figure out we do
not actually need it.
> rs6000-logue.c
> void
> rs6000_emit_prologue (void)
> {
> ...
> bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26840) /* Set frame
> pointer, if needed. */
> bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26841) if
> (frame_pointer_needed)
> bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26842) {
> 0d6c02bf24e4 (jakub 2005-06-30 14:26:32 + 26843) insn =
> emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
> bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26844)
>sp_reg_rtx);
> bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26845)
> RTX_FRAME_RELATED_P (insn) = 1;
> 6b02f2a5c61e (meissner 1995-11-30 20:02:16 + 26846) }
> d1bd513ed578 (kenner 1992-02-09 19:26:21 + 26847)
> ...
> }
Ah, so this you mean. I see. It looks like if you change this to
if (frame_pointer_needed && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM))
{
insn = emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
sp_reg_rtx);
RTX_FRAME_RELATED_P (insn) = 1;
}
(so just that "if" clause changes), it'll all be fine. Could you test
that please?
Thanks,
Segher
Re: [PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true
On 2020/3/27 07:59, Segher Boessenkool wrote:
> Hi!
>
> On Wed, Mar 25, 2020 at 11:15:22PM -0500, luo...@linux.ibm.com wrote:
>> frame_pointer_needed is set to true in reload pass setup_can_eliminate,
>> but regs_ever_live[31] is false, so pro_and_epilogue doesn't save/restore
>> r31 even it is used actually, causing CPU2006 465.tonto segment fault of
>> loading from invalid addresses.
>
> If df_regs_ever_live_p(31) is false there is no hard frame pointer
> anywhere in the program. How can it be used then?
There is a piece of code emit move instruction to r31 even
df_regs_ever_live_p(31) is false
in pro_and_epilogue. As frame_point_needed is true and frame_pointer_needed is
widely
used in this function, so I propose to save r31 in save_reg_p instead of check
(frame_pointer_needed && df_regs_ever_live_p(31), I haven't verify whether this
works yet).
Is this reasonable? Thanks.
rs6000-logue.c
void
rs6000_emit_prologue (void)
{
...
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26840) /* Set frame
pointer, if needed. */
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26841) if
(frame_pointer_needed)
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26842) {
0d6c02bf24e4 (jakub 2005-06-30 14:26:32 + 26843) insn =
emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM),
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26844)
sp_reg_rtx);
bbd21807fdf6 (geoffk 2000-03-16 03:16:41 + 26845)
RTX_FRAME_RELATED_P (insn) = 1;
6b02f2a5c61e (meissner 1995-11-30 20:02:16 + 26846) }
d1bd513ed578 (kenner 1992-02-09 19:26:21 + 26847)
...
}
Xionghu
>
>
> Segher
>
Re: [PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true
Hi! On Wed, Mar 25, 2020 at 11:15:22PM -0500, luo...@linux.ibm.com wrote: > frame_pointer_needed is set to true in reload pass setup_can_eliminate, > but regs_ever_live[31] is false, so pro_and_epilogue doesn't save/restore > r31 even it is used actually, causing CPU2006 465.tonto segment fault of > loading from invalid addresses. If df_regs_ever_live_p(31) is false there is no hard frame pointer anywhere in the program. How can it be used then? Segher
Re: [PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true
On Wed, 2020-03-25 at 23:15 -0500, luoxhu--- via Gcc-patches wrote: > From: Xionghu Luo > Hi, No real issues noted in my review. Patch is straighforward, just a couple cosmetic comments inline below. > This P1 bug is exposed by FRE refactor of r263875. Comparing the fre > dump file shows no obvious change of the segment fault function > proves > it to be a target issue. > frame_pointer_needed is set to true in reload pass > setup_can_eliminate, > but regs_ever_live[31] is false, so pro_and_epilogue doesn't > save/restore > r31 even it is used actually, causing CPU2006 465.tonto segment fault > of > loading from invalid addresses. Could also use a statement here that also reflects what the patch does. "Thus, mark the register as in-use if frame_pointer_needed is true and reg is HARD_FRAME_POINTER_REGNUM." > Bootstrap and regression tested pass on Power8-LE. Backport to gcc-9 > required once approved. > > gcc/ChangeLog > > 2020-03-26 Xiong Hu Luo > > PR target/91518 > * config/rs6000/rs6000-logue.c (save_reg_p): Save r31 if > frame_pointer_needed. We don't actually reference 'r31' in the code change. Maybe preferred to refer to it as HARD_FRAME_POINTER_REGNUM instead? > --- > gcc/config/rs6000/rs6000-logue.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/config/rs6000/rs6000-logue.c > b/gcc/config/rs6000/rs6000-logue.c > index 4cbf228eb79..7b62ff03afd 100644 > --- a/gcc/config/rs6000/rs6000-logue.c > +++ b/gcc/config/rs6000/rs6000-logue.c > @@ -116,6 +116,9 @@ save_reg_p (int reg) > return true; > } > > + if (reg == HARD_FRAME_POINTER_REGNUM && frame_pointer_needed) > +return true; > + Ok. >return !call_used_or_fixed_reg_p (reg) && df_regs_ever_live_p > (reg); > } Thanks -Will
[PATCH] rs6000: Save/restore r31 if frame_pointer_needed is true
From: Xionghu Luo This P1 bug is exposed by FRE refactor of r263875. Comparing the fre dump file shows no obvious change of the segment fault function proves it to be a target issue. frame_pointer_needed is set to true in reload pass setup_can_eliminate, but regs_ever_live[31] is false, so pro_and_epilogue doesn't save/restore r31 even it is used actually, causing CPU2006 465.tonto segment fault of loading from invalid addresses. Bootstrap and regression tested pass on Power8-LE. Backport to gcc-9 required once approved. gcc/ChangeLog 2020-03-26 Xiong Hu Luo PR target/91518 * config/rs6000/rs6000-logue.c (save_reg_p): Save r31 if frame_pointer_needed. --- gcc/config/rs6000/rs6000-logue.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c index 4cbf228eb79..7b62ff03afd 100644 --- a/gcc/config/rs6000/rs6000-logue.c +++ b/gcc/config/rs6000/rs6000-logue.c @@ -116,6 +116,9 @@ save_reg_p (int reg) return true; } + if (reg == HARD_FRAME_POINTER_REGNUM && frame_pointer_needed) +return true; + return !call_used_or_fixed_reg_p (reg) && df_regs_ever_live_p (reg); } -- 2.21.0.777.g83232e3864
