[PATCH 0/2] resolve confilct between RISC-V zcmp and shrink-wrap-separate

[PATCH 0/2] resolve confilct between RISC-V zcmp and shrink-wrap-separate

[Fei Gao]

These 2 patches resolve confilct between zcmp multi push/pop and
shrink-wrap-separate.

As per Kito's review comment
https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg310564.html,
I split the orginal patch into two parts: RISC-V part and
the rest part (shrink-wrap.h / shrink-wrap.cc).

Fei Gao (2):
  allow target to check shrink-wrap-separate enabled or not
  [RISC-V] resolve confilct between zcmp multi push/pop and
    shrink-wrap-separate

 gcc/config/riscv/riscv.cc | 19 ++++++++++++++++---
 gcc/shrink-wrap.cc        | 25 +++++++++++++++++--------
 gcc/shrink-wrap.h         |  1 +
 3 files changed, 34 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH 1/2] allow target to check shrink-wrap-separate enabled or not

[Fei Gao]

gcc/ChangeLog:

        * shrink-wrap.cc (try_shrink_wrapping_separate):call
          use_shrink_wrapping_separate.
        (use_shrink_wrapping_separate): wrap the condition
          check in use_shrink_wrapping_separate.
        * shrink-wrap.h (use_shrink_wrapping_separate): add to extern
---
 gcc/shrink-wrap.cc | 25 +++++++++++++++++--------
 gcc/shrink-wrap.h  |  1 +
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/gcc/shrink-wrap.cc b/gcc/shrink-wrap.cc
index b8d7b557130..d534964321a 100644
--- a/gcc/shrink-wrap.cc
+++ b/gcc/shrink-wrap.cc
@@ -1776,16 +1776,14 @@ insert_prologue_epilogue_for_components (sbitmap components)
   commit_edge_insertions ();
 }
 
-/* The main entry point to this subpass.  FIRST_BB is where the prologue
-   would be normally put.  */
-void
-try_shrink_wrapping_separate (basic_block first_bb)
+bool
+use_shrink_wrapping_separate (void)
 {
   if (!(SHRINK_WRAPPING_ENABLED
-	&& flag_shrink_wrap_separate
-	&& optimize_function_for_speed_p (cfun)
-	&& targetm.shrink_wrap.get_separate_components))
-    return;
+        && flag_shrink_wrap_separate
+        && optimize_function_for_speed_p (cfun)
+        && targetm.shrink_wrap.get_separate_components))
+    return false;
 
   /* We don't handle "strange" functions.  */
   if (cfun->calls_alloca
@@ -1794,6 +1792,17 @@ try_shrink_wrapping_separate (basic_block first_bb)
       || crtl->calls_eh_return
       || crtl->has_nonlocal_goto
       || crtl->saves_all_registers)
+    return false;
+
+  return true;
+}
+
+/* The main entry point to this subpass.  FIRST_BB is where the prologue
+   would be normally put.  */
+void
+try_shrink_wrapping_separate (basic_block first_bb)
+{
+  if (!use_shrink_wrapping_separate ())
     return;
 
   /* Ask the target what components there are.  If it returns NULL, don't
diff --git a/gcc/shrink-wrap.h b/gcc/shrink-wrap.h
index 161647711a3..82386c2b712 100644
--- a/gcc/shrink-wrap.h
+++ b/gcc/shrink-wrap.h
@@ -26,6 +26,7 @@ along with GCC; see the file COPYING3.  If not see
 extern bool requires_stack_frame_p (rtx_insn *, HARD_REG_SET, HARD_REG_SET);
 extern void try_shrink_wrapping (edge *entry_edge, rtx_insn *prologue_seq);
 extern void try_shrink_wrapping_separate (basic_block first_bb);
+extern bool use_shrink_wrapping_separate (void);
 #define SHRINK_WRAPPING_ENABLED \
   (flag_shrink_wrap && targetm.have_simple_return ())
 
-- 
2.17.1

Re: [PATCH 1/2] allow target to check shrink-wrap-separate enabled or not

[Jeff Law]

On 6/20/23 03:40, Fei Gao wrote:
> gcc/ChangeLog:
> 
>          * shrink-wrap.cc (try_shrink_wrapping_separate):call
>            use_shrink_wrapping_separate.
>          (use_shrink_wrapping_separate): wrap the condition
>            check in use_shrink_wrapping_separate.
>          * shrink-wrap.h (use_shrink_wrapping_separate): add to extern
I'm still missing somethign here.

Why doesn't the RISC-V target simply disable separate shrink wrapping by 
indicating no components are eligible in the relevant cases.  ie, I do 
not think we need another knob here.

To be more concrete:

> /* Implement TARGET_SHRINK_WRAP_GET_SEPARATE_COMPONENTS.  */
>   
> static sbitmap
> riscv_get_separate_components (void)
> {       
>   HOST_WIDE_INT offset;
>   sbitmap components = sbitmap_alloc (FIRST_PSEUDO_REGISTER);
>   bitmap_clear (components);
>           
>   if (riscv_use_save_libcall (&cfun->machine->frame)
>       || cfun->machine->interrupt_handler_p
>       || !cfun->machine->frame.gp_sp_offset.is_constant ())
>     return components;
Don't we get the behavior we want if we change this code to return an 
zero'd sbitmap?

jeff

Re: Re: [PATCH 1/2] allow target to check shrink-wrap-separate enabled or not

[Fei Gao]

hi Jeff

Please see my earlier reply here.
https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg310656.html

Maybe you scrolled past it in so many emails:)

BR, 
Fei


On 2023-06-25 21:36  Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
>On 6/20/23 03:40, Fei Gao wrote:
>> gcc/ChangeLog:
>>
>>          * shrink-wrap.cc (try_shrink_wrapping_separate):call
>>            use_shrink_wrapping_separate.
>>          (use_shrink_wrapping_separate): wrap the condition
>>            check in use_shrink_wrapping_separate.
>>          * shrink-wrap.h (use_shrink_wrapping_separate): add to extern
>I'm still missing somethign here.
>
>Why doesn't the RISC-V target simply disable separate shrink wrapping by
>indicating no components are eligible in the relevant cases.  ie, I do
>not think we need another knob here.
>
>To be more concrete:
>
>> /* Implement TARGET_SHRINK_WRAP_GET_SEPARATE_COMPONENTS.  */
>>  
>> static sbitmap
>> riscv_get_separate_components (void)
>> {      
>>   HOST_WIDE_INT offset;
>>   sbitmap components = sbitmap_alloc (FIRST_PSEUDO_REGISTER);
>>   bitmap_clear (components);
>>          
>>   if (riscv_use_save_libcall (&cfun->machine->frame)
>>       || cfun->machine->interrupt_handler_p
>>       || !cfun->machine->frame.gp_sp_offset.is_constant ())
>>     return components;
>Don't we get the behavior we want if we change this code to return an
>zero'd sbitmap?
>
>jeff

Re: [PATCH 1/2] allow target to check shrink-wrap-separate enabled or not

[Jeff Law]

On 6/25/23 20:29, Fei Gao wrote:
> hi Jeff
> 
> Please see my earlier reply here.
> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg310656.html
> 
> Maybe you scrolled past it in so many emails:)
It definitely got lost in my mountain of mail.

jeff

Re: [PATCH 1/2] allow target to check shrink-wrap-separate enabled or not

[Jeff Law]

On 6/25/23 20:29, Fei Gao wrote:
> hi Jeff
> 
> Please see my earlier reply here.
> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg310656.html
> 
> Maybe you scrolled past it in so many emails:)
Oh, so the issue isn't really the set of components being wrapped, but 
the way in which we save them.  Yea, that's going to need some tinkering.

It does make me wonder if we can handle this in riscv_override_options. 
That's a pretty standard place to deal with option conflicts.  We ought 
to be able to check if both options are enabled, then disable zcmp 
push/pop at that poing without introducing any new hooks.


jeff

[PATCH 2/2] [RISC-V] resolve confilct between zcmp multi push/pop and shrink-wrap-separate

[Fei Gao]

Disable zcmp multi push/pop if shrink-wrap-separate is active.

So in -Os that prefers smaller code size, by default shrink-wrap-separate
is disabled while zcmp multi push/pop is enabled.

And in -O2 and others that prefers speed, by default shrink-wrap-separate
is enabled while zcmp multi push/pop is disabled. To force enabling zcmp multi
push/pop in this case, -fno-shrink-wrap-separate has to be explictly given.

The following TC shows the issues in -O2 before this patch with both
shrink-wrap-separate and zcmp multi push/pop active.
1. duplicated store of s regs.
2. cm.push pushes ra, s0-s11 in reverse order than what normal
   prologue does, causing stack corruption and failure to resotre s regs.

TC: zcmp_shrink_wrap_separate.c included in this patch.

output asm before this patch:
calc_func:
	cm.push	{ra, s0-s3}, -32
	...
	beq	a5,zero,.L2
	...
.L2:
	...
	sw	s1,20(sp) //issue here
	sw	s3,12(sp) //issue here
	...
	sw	s2,16(sp) //issue here

output asm after this patch:
calc_func:
	addi	sp,sp,-32
	sw	s0,24(sp)
	...
	beq	a5,zero,.L2
	...
.L2:
	...
	sw	s1,20(sp)
	sw	s3,12(sp)
	...
	sw	s2,16(sp)

Signed-off-by: Fei Gao <gaofei@eswincomputing.com>
Co-Authored-By: Zhangjin Liao <liaozhangjin@eswincomputing.com>

gcc/ChangeLog:

        * config/riscv/riscv.cc
        (riscv_avoid_shrink_wrapping_separate): wrap the condition check in
          riscv_avoid_shrink_wrapping_separate.
        (riscv_avoid_multi_push): avoid multi push if shrink_wrapping_separate
          is active.
        (riscv_get_separate_components): call riscv_avoid_shrink_wrapping_separate
---
 gcc/config/riscv/riscv.cc | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 26405b5978b..2cca5fbb62d 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -64,6 +64,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "cfghooks.h"
 #include "cfgloop.h"
 #include "cfgrtl.h"
+#include "shrink-wrap.h"
 #include "sel-sched.h"
 #include "fold-const.h"
 #include "gimple-iterator.h"
@@ -389,6 +390,7 @@ static const struct riscv_tune_param optimize_size_tune_info = {
   false,					/* use_divmod_expansion */
 };
 
+static bool riscv_avoid_shrink_wrapping_separate ();
 static tree riscv_handle_fndecl_attribute (tree *, tree, tree, int, bool *);
 static tree riscv_handle_type_attribute (tree *, tree, tree, int, bool *);
 
@@ -5032,6 +5034,8 @@ riscv_avoid_multi_push(const struct riscv_frame_info *frame)
       || cfun->machine->interrupt_handler_p
       || cfun->machine->varargs_size != 0
       || crtl->args.pretend_args_size != 0
+      || (use_shrink_wrapping_separate ()
+          && !riscv_avoid_shrink_wrapping_separate ())
       || (frame->mask & ~ MULTI_PUSH_GPR_MASK))
     return true;
 
@@ -6199,6 +6203,17 @@ riscv_epilogue_uses (unsigned int regno)
   return false;
 }
 
+static bool
+riscv_avoid_shrink_wrapping_separate ()
+{
+  if (riscv_use_save_libcall (&cfun->machine->frame)
+      || cfun->machine->interrupt_handler_p
+      || !cfun->machine->frame.gp_sp_offset.is_constant ())
+    return true;
+
+  return false;
+}
+
 /* Implement TARGET_SHRINK_WRAP_GET_SEPARATE_COMPONENTS.  */
 
 static sbitmap
@@ -6208,9 +6223,7 @@ riscv_get_separate_components (void)
   sbitmap components = sbitmap_alloc (FIRST_PSEUDO_REGISTER);
   bitmap_clear (components);
 
-  if (riscv_use_save_libcall (&cfun->machine->frame)
-      || cfun->machine->interrupt_handler_p
-      || !cfun->machine->frame.gp_sp_offset.is_constant ())
+  if (riscv_avoid_shrink_wrapping_separate ())
     return components;
 
   offset = cfun->machine->frame.gp_sp_offset.to_constant ();
-- 
2.17.1