| Message ID | 20190325110313.9271-2-claziss@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve register elimination, and other LRA | expand |
* Claudiu Zissulescu <claziss@gmail.com> [2019-03-25 12:03:11 +0100]: > 1.The delay slot scheduler can reschedule some of the frame related > instructions resulting in having incorect CFI information. This patch > introduces a schedule blockage to avoid this problem. > > 2.There are cases when an interrupt may happen and not all the current > function stack operations are done, which may result in stack > corruption. Such an example is accessing an returning a local > structure members, which members are allocated on stack. The stack > adjustment and the accessing of the struct member can be reorder as > they may not use both the SP register for the access. > > 3.Also, do not save/restore SP when in interrupt. The SP is switch by > the core IRQ machinery. > > gcc/ > xxxx-xx-xx Claudiu Zissulescu <claziss@synopsys.com> > > * config/arc/arc.c (arc_expand_prologue): Emit blockage regardless > to avoid delay slot scheduling. > * config/arc/arc.md (stack_tie): Remove. > (UNSPEC_ARC_STKTIE): Likewise. > (arc_must_save_register): Don't save SP. > (arc_expand_epilogue): Emit blockage. This looks fine, just one minor issue... > --- > gcc/config/arc/arc.c | 17 +++++++++-------- > gcc/config/arc/arc.md | 13 ------------- > 2 files changed, 9 insertions(+), 21 deletions(-) > > diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c > index fa49c562b46..62f435b0a1d 100644 > --- a/gcc/config/arc/arc.c > +++ b/gcc/config/arc/arc.c > @@ -2658,6 +2658,7 @@ arc_must_save_register (int regno, struct function *func) > > if ((regno) != RETURN_ADDR_REGNUM > && (regno) != FRAME_POINTER_REGNUM > + && (regno) != STACK_POINTER_REGNUM > && df_regs_ever_live_p (regno) > && (!call_used_regs[regno] > || ARC_INTERRUPT_P (fn_type)) The comment at the head of this function mentions the special handling for ra and fp, but wasn't updated with this change. Would it make sense to do that? Thanks, Andrew > @@ -3731,14 +3732,10 @@ arc_expand_prologue (void) > > /* Allocate the stack frame. */ > if (frame_size_to_allocate > 0) > - { > - frame_stack_add ((HOST_WIDE_INT) 0 - frame_size_to_allocate); > - /* If the frame pointer is needed, emit a special barrier that > - will prevent the scheduler from moving stores to the frame > - before the stack adjustment. */ > - if (arc_frame_pointer_needed ()) > - emit_insn (gen_stack_tie (stack_pointer_rtx, hard_frame_pointer_rtx)); > - } > + frame_stack_add ((HOST_WIDE_INT) 0 - frame_size_to_allocate); > + > + /* Emit a blockage to avoid delay slot scheduling. */ > + emit_insn (gen_blockage ()); > } > > /* Do any necessary cleanup after a function to restore stack, frame, > @@ -3775,6 +3772,10 @@ arc_expand_epilogue (int sibcall_p) > if (!can_trust_sp_p) > gcc_assert (arc_frame_pointer_needed ()); > > + /* Emit a blockage to avoid/flush all pending sp operations. */ > + if (size) > + emit_insn (gen_blockage ()); > + > if (TARGET_CODE_DENSITY > && TARGET_CODE_DENSITY_FRAME > && !ARC_AUTOFP_IRQ_P (fn_type) > diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md > index 0482980122d..3a903d4224a 100644 > --- a/gcc/config/arc/arc.md > +++ b/gcc/config/arc/arc.md > @@ -136,7 +136,6 @@ > UNSPEC_ARC_VMAC2HU > UNSPEC_ARC_VMPY2H > UNSPEC_ARC_VMPY2HU > - UNSPEC_ARC_STKTIE > > VUNSPEC_ARC_RTIE > VUNSPEC_ARC_SYNC > @@ -6301,18 +6300,6 @@ core_3, archs4x, archs4xd, archs4xd_slow" > (set_attr "predicable" "yes,no,no,yes,no") > (set_attr "cond" "canuse,nocond,nocond,canuse_limm,nocond")]) > > -(define_insn "stack_tie" > - [(set (mem:BLK (scratch)) > - (unspec:BLK [(match_operand:SI 0 "register_operand" "r") > - (match_operand:SI 1 "register_operand" "r")] > - UNSPEC_ARC_STKTIE))] > - "" > - "" > - [(set_attr "length" "0") > - (set_attr "iscompact" "false") > - (set_attr "type" "block")] > - ) > - > (define_insn "*add_shift" > [(set (match_operand:SI 0 "register_operand" "=q,r,r") > (plus:SI (ashift:SI (match_operand:SI 1 "register_operand" "q,r,r") > -- > 2.20.1 >
Committed with the suggested info. Thank you for your review, Claudiu On Thu, Apr 11, 2019 at 12:47 AM Andrew Burgess <andrew.burgess@embecosm.com> wrote: > > * Claudiu Zissulescu <claziss@gmail.com> [2019-03-25 12:03:11 +0100]: > > > 1.The delay slot scheduler can reschedule some of the frame related > > instructions resulting in having incorect CFI information. This patch > > introduces a schedule blockage to avoid this problem. > > > > 2.There are cases when an interrupt may happen and not all the current > > function stack operations are done, which may result in stack > > corruption. Such an example is accessing an returning a local > > structure members, which members are allocated on stack. The stack > > adjustment and the accessing of the struct member can be reorder as > > they may not use both the SP register for the access. > > > > 3.Also, do not save/restore SP when in interrupt. The SP is switch by > > the core IRQ machinery. > > > > gcc/ > > xxxx-xx-xx Claudiu Zissulescu <claziss@synopsys.com> > > > > * config/arc/arc.c (arc_expand_prologue): Emit blockage regardless > > to avoid delay slot scheduling. > > * config/arc/arc.md (stack_tie): Remove. > > (UNSPEC_ARC_STKTIE): Likewise. > > (arc_must_save_register): Don't save SP. > > (arc_expand_epilogue): Emit blockage. > > This looks fine, just one minor issue... > > > --- > > gcc/config/arc/arc.c | 17 +++++++++-------- > > gcc/config/arc/arc.md | 13 ------------- > > 2 files changed, 9 insertions(+), 21 deletions(-) > > > > diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c > > index fa49c562b46..62f435b0a1d 100644 > > --- a/gcc/config/arc/arc.c > > +++ b/gcc/config/arc/arc.c > > @@ -2658,6 +2658,7 @@ arc_must_save_register (int regno, struct function *func) > > > > if ((regno) != RETURN_ADDR_REGNUM > > && (regno) != FRAME_POINTER_REGNUM > > + && (regno) != STACK_POINTER_REGNUM > > && df_regs_ever_live_p (regno) > > && (!call_used_regs[regno] > > || ARC_INTERRUPT_P (fn_type)) > > The comment at the head of this function mentions the special handling > for ra and fp, but wasn't updated with this change. Would it make > sense to do that? > > Thanks, > Andrew > > > @@ -3731,14 +3732,10 @@ arc_expand_prologue (void) > > > > /* Allocate the stack frame. */ > > if (frame_size_to_allocate > 0) > > - { > > - frame_stack_add ((HOST_WIDE_INT) 0 - frame_size_to_allocate); > > - /* If the frame pointer is needed, emit a special barrier that > > - will prevent the scheduler from moving stores to the frame > > - before the stack adjustment. */ > > - if (arc_frame_pointer_needed ()) > > - emit_insn (gen_stack_tie (stack_pointer_rtx, hard_frame_pointer_rtx)); > > - } > > + frame_stack_add ((HOST_WIDE_INT) 0 - frame_size_to_allocate); > > + > > + /* Emit a blockage to avoid delay slot scheduling. */ > > + emit_insn (gen_blockage ()); > > } > > > > /* Do any necessary cleanup after a function to restore stack, frame, > > @@ -3775,6 +3772,10 @@ arc_expand_epilogue (int sibcall_p) > > if (!can_trust_sp_p) > > gcc_assert (arc_frame_pointer_needed ()); > > > > + /* Emit a blockage to avoid/flush all pending sp operations. */ > > + if (size) > > + emit_insn (gen_blockage ()); > > + > > if (TARGET_CODE_DENSITY > > && TARGET_CODE_DENSITY_FRAME > > && !ARC_AUTOFP_IRQ_P (fn_type) > > diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md > > index 0482980122d..3a903d4224a 100644 > > --- a/gcc/config/arc/arc.md > > +++ b/gcc/config/arc/arc.md > > @@ -136,7 +136,6 @@ > > UNSPEC_ARC_VMAC2HU > > UNSPEC_ARC_VMPY2H > > UNSPEC_ARC_VMPY2HU > > - UNSPEC_ARC_STKTIE > > > > VUNSPEC_ARC_RTIE > > VUNSPEC_ARC_SYNC > > @@ -6301,18 +6300,6 @@ core_3, archs4x, archs4xd, archs4xd_slow" > > (set_attr "predicable" "yes,no,no,yes,no") > > (set_attr "cond" "canuse,nocond,nocond,canuse_limm,nocond")]) > > > > -(define_insn "stack_tie" > > - [(set (mem:BLK (scratch)) > > - (unspec:BLK [(match_operand:SI 0 "register_operand" "r") > > - (match_operand:SI 1 "register_operand" "r")] > > - UNSPEC_ARC_STKTIE))] > > - "" > > - "" > > - [(set_attr "length" "0") > > - (set_attr "iscompact" "false") > > - (set_attr "type" "block")] > > - ) > > - > > (define_insn "*add_shift" > > [(set (match_operand:SI 0 "register_operand" "=q,r,r") > > (plus:SI (ashift:SI (match_operand:SI 1 "register_operand" "q,r,r") > > -- > > 2.20.1 > >
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c index fa49c562b46..62f435b0a1d 100644 --- a/gcc/config/arc/arc.c +++ b/gcc/config/arc/arc.c @@ -2658,6 +2658,7 @@ arc_must_save_register (int regno, struct function *func) if ((regno) != RETURN_ADDR_REGNUM && (regno) != FRAME_POINTER_REGNUM + && (regno) != STACK_POINTER_REGNUM && df_regs_ever_live_p (regno) && (!call_used_regs[regno] || ARC_INTERRUPT_P (fn_type)) @@ -3731,14 +3732,10 @@ arc_expand_prologue (void) /* Allocate the stack frame. */ if (frame_size_to_allocate > 0) - { - frame_stack_add ((HOST_WIDE_INT) 0 - frame_size_to_allocate); - /* If the frame pointer is needed, emit a special barrier that - will prevent the scheduler from moving stores to the frame - before the stack adjustment. */ - if (arc_frame_pointer_needed ()) - emit_insn (gen_stack_tie (stack_pointer_rtx, hard_frame_pointer_rtx)); - } + frame_stack_add ((HOST_WIDE_INT) 0 - frame_size_to_allocate); + + /* Emit a blockage to avoid delay slot scheduling. */ + emit_insn (gen_blockage ()); } /* Do any necessary cleanup after a function to restore stack, frame, @@ -3775,6 +3772,10 @@ arc_expand_epilogue (int sibcall_p) if (!can_trust_sp_p) gcc_assert (arc_frame_pointer_needed ()); + /* Emit a blockage to avoid/flush all pending sp operations. */ + if (size) + emit_insn (gen_blockage ()); + if (TARGET_CODE_DENSITY && TARGET_CODE_DENSITY_FRAME && !ARC_AUTOFP_IRQ_P (fn_type) diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md index 0482980122d..3a903d4224a 100644 --- a/gcc/config/arc/arc.md +++ b/gcc/config/arc/arc.md @@ -136,7 +136,6 @@ UNSPEC_ARC_VMAC2HU UNSPEC_ARC_VMPY2H UNSPEC_ARC_VMPY2HU - UNSPEC_ARC_STKTIE VUNSPEC_ARC_RTIE VUNSPEC_ARC_SYNC @@ -6301,18 +6300,6 @@ core_3, archs4x, archs4xd, archs4xd_slow" (set_attr "predicable" "yes,no,no,yes,no") (set_attr "cond" "canuse,nocond,nocond,canuse_limm,nocond")]) -(define_insn "stack_tie" - [(set (mem:BLK (scratch)) - (unspec:BLK [(match_operand:SI 0 "register_operand" "r") - (match_operand:SI 1 "register_operand" "r")] - UNSPEC_ARC_STKTIE))] - "" - "" - [(set_attr "length" "0") - (set_attr "iscompact" "false") - (set_attr "type" "block")] - ) - (define_insn "*add_shift" [(set (match_operand:SI 0 "register_operand" "=q,r,r") (plus:SI (ashift:SI (match_operand:SI 1 "register_operand" "q,r,r")