[6/7,ARC] Prevent moving stores to the frame before the stack adjustment.

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(-)

* 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