[1/3,ARC] Emit blockage regardless to avoid delay slot scheduling.

Message ID 20190325110313.9271-2-claziss@gmail.com
State New
Headers show
Series
  • Improve register elimination, and other LRA
Related show

Commit Message

Claudiu Zissulescu March 25, 2019, 11:03 a.m.
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.
---
 gcc/config/arc/arc.c  | 17 +++++++++--------
 gcc/config/arc/arc.md | 13 -------------
 2 files changed, 9 insertions(+), 21 deletions(-)

-- 
2.20.1

Comments

Andrew Burgess April 10, 2019, 9:47 p.m. | #1
* 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

>
Claudiu Zissulescu April 16, 2019, 10:22 a.m. | #2
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

> >

Patch

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