Message ID | 1495189862-20533-7-git-send-email-claziss@synopsys.com |
---|---|
State | New |
Headers | show |
* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2017-05-19 12:31:01 +0200]: > From: Claudiu Zissulescu <claziss@gmail.com> > > If the stack pointer is needed, emit a special barrier that will prevent > the scheduler from moving stores to the frame before the stack adjustment. > > 2017-01-03 Claudiu Zissulescu <claziss@synopsys.com> > > * config/arc/arc.c (arc_expand_prologue): Emit a special barrier > to prevent store reordering. > * config/arc/arc.md (UNSPEC_ARC_STKTIE): Define. > (type): Add block type. > (stack_tie): Define special instruction to be used in > expand_prologue. Given the description the code looks fine. It would be nice to see more of a _why_ in the commit message. I'm guessing this is either something related to signal handling, or debugging... I don't see why this would be needed for functional correctness. Thanks, Andrew > --- > gcc/config/arc/arc.c | 10 +++++++++- > gcc/config/arc/arc.md | 15 ++++++++++++++- > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c > index ff86f6c..0c4c901 100644 > --- a/gcc/config/arc/arc.c > +++ b/gcc/config/arc/arc.c > @@ -3030,7 +3030,15 @@ arc_expand_prologue (void) > frame_size_to_allocate -= first_offset; > /* Allocate the stack frame. */ > if (frame_size_to_allocate > 0) > - frame_stack_add ((HOST_WIDE_INT) 0 - frame_size_to_allocate); > + { > + 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)); > + } > > /* Setup the gp register, if needed. */ > if (crtl->uses_pic_offset_table) > diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md > index 743a844..6cd192a 100644 > --- a/gcc/config/arc/arc.md > +++ b/gcc/config/arc/arc.md > @@ -135,6 +135,7 @@ > UNSPEC_ARC_VMAC2HU > UNSPEC_ARC_VMPY2H > UNSPEC_ARC_VMPY2HU > + UNSPEC_ARC_STKTIE > ]) > > (define_c_enum "vunspec" [ > @@ -205,7 +206,7 @@ > simd_vcompare, simd_vpermute, simd_vpack, simd_vpack_with_acc, > simd_valign, simd_valign_with_acc, simd_vcontrol, > simd_vspecial_3cycle, simd_vspecial_4cycle, simd_dma, mul16_em, div_rem, > - fpu" > + fpu, block" > (cond [(eq_attr "is_sfunc" "yes") > (cond [(match_test "!TARGET_LONG_CALLS_SET && (!TARGET_MEDIUM_CALLS || GET_CODE (PATTERN (insn)) != COND_EXEC)") (const_string "call") > (match_test "flag_pic") (const_string "sfunc")] > @@ -6547,6 +6548,18 @@ > (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" "rb") > + (match_operand:SI 1 "register_operand" "rb")] > + UNSPEC_ARC_STKTIE))] > + "" > + "" > + [(set_attr "length" "0") > + (set_attr "iscompact" "false") > + (set_attr "type" "block")] > + ) > + > ;; include the arc-FPX instructions > (include "fpx.md") > > -- > 1.9.1 >
> Given the description the code looks fine. It would be nice to see > more of a _why_ in the commit message. I'm guessing this is either > something related to signal handling, or debugging... I don't see why > this would be needed for functional correctness. > The issue is how we generate a function prologue when we use fno-omit-frame-pointer, for example: [snip] mov_s fp,sp ; frame pointer is set here [snip] st r1,[fp,-24] ; frame pointer is used here [snip] sub_s sp,sp,0x20 ; stack pointer adjusted So we can easily see that any interrupt between the `st` and `sub` instruction will lead to faulty code as the interrupt routine will use a faulty sp register, and, potentially, overwriting the value stored by 'st' instruction. Thus, adding a scheduler barrier will force the compiler to emit the `sub` instruction before the store one. Thanks, Claudiu
> Given the description the code looks fine. It would be nice to see > more of a _why_ in the commit message. I'm guessing this is either > something related to signal handling, or debugging... I don't see why > this would be needed for functional correctness. Committed with additional comments. Thank you, Claudiu
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c index ff86f6c..0c4c901 100644 --- a/gcc/config/arc/arc.c +++ b/gcc/config/arc/arc.c @@ -3030,7 +3030,15 @@ arc_expand_prologue (void) frame_size_to_allocate -= first_offset; /* Allocate the stack frame. */ if (frame_size_to_allocate > 0) - frame_stack_add ((HOST_WIDE_INT) 0 - frame_size_to_allocate); + { + 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)); + } /* Setup the gp register, if needed. */ if (crtl->uses_pic_offset_table) diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md index 743a844..6cd192a 100644 --- a/gcc/config/arc/arc.md +++ b/gcc/config/arc/arc.md @@ -135,6 +135,7 @@ UNSPEC_ARC_VMAC2HU UNSPEC_ARC_VMPY2H UNSPEC_ARC_VMPY2HU + UNSPEC_ARC_STKTIE ]) (define_c_enum "vunspec" [ @@ -205,7 +206,7 @@ simd_vcompare, simd_vpermute, simd_vpack, simd_vpack_with_acc, simd_valign, simd_valign_with_acc, simd_vcontrol, simd_vspecial_3cycle, simd_vspecial_4cycle, simd_dma, mul16_em, div_rem, - fpu" + fpu, block" (cond [(eq_attr "is_sfunc" "yes") (cond [(match_test "!TARGET_LONG_CALLS_SET && (!TARGET_MEDIUM_CALLS || GET_CODE (PATTERN (insn)) != COND_EXEC)") (const_string "call") (match_test "flag_pic") (const_string "sfunc")] @@ -6547,6 +6548,18 @@ (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" "rb") + (match_operand:SI 1 "register_operand" "rb")] + UNSPEC_ARC_STKTIE))] + "" + "" + [(set_attr "length" "0") + (set_attr "iscompact" "false") + (set_attr "type" "block")] + ) + ;; include the arc-FPX instructions (include "fpx.md")
From: Claudiu Zissulescu <claziss@gmail.com> If the stack pointer is needed, emit a special barrier that will prevent the scheduler from moving stores to the frame before the stack adjustment. 2017-01-03 Claudiu Zissulescu <claziss@synopsys.com> * config/arc/arc.c (arc_expand_prologue): Emit a special barrier to prevent store reordering. * config/arc/arc.md (UNSPEC_ARC_STKTIE): Define. (type): Add block type. (stack_tie): Define special instruction to be used in expand_prologue. --- gcc/config/arc/arc.c | 10 +++++++++- gcc/config/arc/arc.md | 15 ++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-)