diff mbox series

[1/2,RISC-V] disable shrink-wrap-separate if zcmp enabled.

Message ID 20230506083939.22097-2-gaofei@eswincomputing.com
State Accepted
Headers
Series RISC-V: support Zcmp extension |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Fei Gao May 6, 2023, 8:39 a.m. UTC 
  zcmp aims to reduce code size, while shrink-wrap-separate prefers
speed to code size. So disable shrink-wrap-separate if zcmp
enabled, just like what save-restore has done.

author: Zhangjin Liao liaozhangjin@eswincomputing.com

gcc/ChangeLog:

        * config/riscv/riscv.cc (riscv_get_separate_components):
---
 gcc/config/riscv/riscv.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kito Cheng May 8, 2023, 2:41 a.m. UTC | #1 
shrink-wraping already gated by Os so I think we don't need add more
gate here, unless we are trying to claim force optimize for size if
zcmp is present.

On Sat, May 6, 2023 at 4:41 PM Fei Gao <gaofei@eswincomputing.com> wrote:
>
> zcmp aims to reduce code size, while shrink-wrap-separate prefers
> speed to code size. So disable shrink-wrap-separate if zcmp
> enabled, just like what save-restore has done.
>
> author: Zhangjin Liao liaozhangjin@eswincomputing.com
>
> gcc/ChangeLog:
>
>         * config/riscv/riscv.cc (riscv_get_separate_components):
> ---
>  gcc/config/riscv/riscv.cc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 45a63cab9c9..629e5e45cac 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -5729,7 +5729,8 @@ riscv_get_separate_components (void)
>
>    if (riscv_use_save_libcall (&cfun->machine->frame)
>        || cfun->machine->interrupt_handler_p
> -      || !cfun->machine->frame.gp_sp_offset.is_constant ())
> +      || !cfun->machine->frame.gp_sp_offset.is_constant ()
> +      || TARGET_ZCMP)
>      return components;
>
>    offset = cfun->machine->frame.gp_sp_offset.to_constant ();
> --
> 2.17.1
>
Fei Gao May 8, 2023, 7:39 a.m. UTC | #2 
On 2023-05-08 10:41  Kito Cheng <kito.cheng@gmail.com> wrote:
>
>shrink-wraping already gated by Os so I think we don't need add more
>gate here, unless we are trying to claim force optimize for size if
>zcmp is present.
> 

hi Kito

Zcmp is added here just like save-restore.

Either we add them both, or delete.

BR, 
Fei

>On Sat, May 6, 2023 at 4:41 PM Fei Gao <gaofei@eswincomputing.com> wrote:
>>
>> zcmp aims to reduce code size, while shrink-wrap-separate prefers
>> speed to code size. So disable shrink-wrap-separate if zcmp
>> enabled, just like what save-restore has done.
>>
>> author: Zhangjin Liao liaozhangjin@eswincomputing.com
>>
>> gcc/ChangeLog:
>>
>>         * config/riscv/riscv.cc (riscv_get_separate_components):
>> ---
>>  gcc/config/riscv/riscv.cc | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 45a63cab9c9..629e5e45cac 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -5729,7 +5729,8 @@ riscv_get_separate_components (void)
>>
>>    if (riscv_use_save_libcall (&cfun->machine->frame)
>>        || cfun->machine->interrupt_handler_p
>> -      || !cfun->machine->frame.gp_sp_offset.is_constant ())
>> +      || !cfun->machine->frame.gp_sp_offset.is_constant ()
>> +      || TARGET_ZCMP)
>>      return components;
>>
>>    offset = cfun->machine->frame.gp_sp_offset.to_constant ();
>> --
>> 2.17.1
>>
Andrew Pinski May 8, 2023, 7:54 a.m. UTC | #3 
On Sat, May 6, 2023 at 1:41 AM Fei Gao <gaofei@eswincomputing.com> wrote:
>
> zcmp aims to reduce code size, while shrink-wrap-separate prefers
> speed to code size. So disable shrink-wrap-separate if zcmp
> enabled, just like what save-restore has done.
>
> author: Zhangjin Liao liaozhangjin@eswincomputing.com
>
> gcc/ChangeLog:
>
>         * config/riscv/riscv.cc (riscv_get_separate_components):
> ---
>  gcc/config/riscv/riscv.cc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 45a63cab9c9..629e5e45cac 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -5729,7 +5729,8 @@ riscv_get_separate_components (void)
>
>    if (riscv_use_save_libcall (&cfun->machine->frame)
>        || cfun->machine->interrupt_handler_p
> -      || !cfun->machine->frame.gp_sp_offset.is_constant ())
> +      || !cfun->machine->frame.gp_sp_offset.is_constant ()
> +      || TARGET_ZCMP)
>      return components;

I think this is a bad idea. I have a use case where we use the C
extensions but still compile for -O2 because we want the code to be
fast as possible but still having the savings of the C extensions.

Thanks,
Andrew Pinski

>
>    offset = cfun->machine->frame.gp_sp_offset.to_constant ();
> --
> 2.17.1
>
Kito Cheng May 8, 2023, 8:05 a.m. UTC | #4 
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 45a63cab9c9..629e5e45cac 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -5729,7 +5729,8 @@ riscv_get_separate_components (void)
> >
> >    if (riscv_use_save_libcall (&cfun->machine->frame)
> >        || cfun->machine->interrupt_handler_p
> > -      || !cfun->machine->frame.gp_sp_offset.is_constant ())
> > +      || !cfun->machine->frame.gp_sp_offset.is_constant ()
> > +      || TARGET_ZCMP)
> >      return components;
>
> I think this is a bad idea. I have a use case where we use the C
> extensions but still compile for -O2 because we want the code to be
> fast as possible but still having the savings of the C extensions.

Yeah, agree, so I would prefer to drop this from the patch series.

> Thanks,
> Andrew Pinski
Fei Gao May 8, 2023, 8:54 a.m. UTC | #5 
On 2023-05-08 16:05  Kito Cheng <kito.cheng@gmail.com> wrote:
>
>> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> > index 45a63cab9c9..629e5e45cac 100644
>> > --- a/gcc/config/riscv/riscv.cc
>> > +++ b/gcc/config/riscv/riscv.cc
>> > @@ -5729,7 +5729,8 @@ riscv_get_separate_components (void)
>> >
>> >    if (riscv_use_save_libcall (&cfun->machine->frame)
>> >        || cfun->machine->interrupt_handler_p
>> > -      || !cfun->machine->frame.gp_sp_offset.is_constant ())
>> > +      || !cfun->machine->frame.gp_sp_offset.is_constant ()
>> > +      || TARGET_ZCMP)
>> >      return components;
>>
>> I think this is a bad idea. I have a use case where we use the C
>> extensions but still compile for -O2 because we want the code to be
>> fast as possible but still having the savings of the C extensions.
>
>Yeah, agree, so I would prefer to drop this from the patch series. 

Zcmp is a little different here than C. 
C extension is done fully in AS.  So  we have the code to be
fast as possible but still having the savings of the C extensions.

Zcmp and shrink-wrap-separate are both done in prologue/epilogue pass
and you can only have one switch active to direct sregs save and restore.
In my understanding, zcmp push and pop insns seem to
be mutually exclusive in functionality to shrink-wrap-separate. 
It's not expected to see zcmp insns at the begining/end of prologue/epilogue, 
and also repeated store/load sregs in separate blocks.  

Same for save and restore, and i guess that's why we have 
riscv_use_save_libcall (&cfun->machine->frame) check here.

BR, 
Fei

>
>> Thanks,
>> Andrew Pinski
Kito Cheng May 8, 2023, 9:20 a.m. UTC | #6 
-msave-restore is a different story; it's only enabled when the user
requests, but `-march` describes the capability of the target
architecture, not specify the preference of performance or size, which
should be determined by -O1~-O3/-Ofast or -Os/-Oz.

On Mon, May 8, 2023 at 4:54 PM Fei Gao <gaofei@eswincomputing.com> wrote:
>
> On 2023-05-08 16:05  Kito Cheng <kito.cheng@gmail.com> wrote:
> >
> >> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> >> > index 45a63cab9c9..629e5e45cac 100644
> >> > --- a/gcc/config/riscv/riscv.cc
> >> > +++ b/gcc/config/riscv/riscv.cc
> >> > @@ -5729,7 +5729,8 @@ riscv_get_separate_components (void)
> >> >
> >> >    if (riscv_use_save_libcall (&cfun->machine->frame)
> >> >        || cfun->machine->interrupt_handler_p
> >> > -      || !cfun->machine->frame.gp_sp_offset.is_constant ())
> >> > +      || !cfun->machine->frame.gp_sp_offset.is_constant ()
> >> > +      || TARGET_ZCMP)
> >> >      return components;
> >>
> >> I think this is a bad idea. I have a use case where we use the C
> >> extensions but still compile for -O2 because we want the code to be
> >> fast as possible but still having the savings of the C extensions.
> >
> >Yeah, agree, so I would prefer to drop this from the patch series.
>
> Zcmp is a little different here than C.
> C extension is done fully in AS.  So  we have the code to be
> fast as possible but still having the savings of the C extensions.
>
> Zcmp and shrink-wrap-separate are both done in prologue/epilogue pass
> and you can only have one switch active to direct sregs save and restore.
> In my understanding, zcmp push and pop insns seem to
> be mutually exclusive in functionality to shrink-wrap-separate.
> It's not expected to see zcmp insns at the begining/end of prologue/epilogue,
> and also repeated store/load sregs in separate blocks.
>
> Same for save and restore, and i guess that's why we have
> riscv_use_save_libcall (&cfun->machine->frame) check here.
>
> BR,
> Fei
>
> >
> >> Thanks,
> >> Andrew Pinski
Fei Gao May 10, 2023, 9:33 a.m. UTC | #7 
On 2023-05-08 17:20  Kito Cheng <kito.cheng@gmail.com> wrote:
>
>-msave-restore is a different story; it's only enabled when the user
>requests, but `-march` describes the capability of the target
>architecture, not specify the preference of performance or size, which
>should be determined by -O1~-O3/-Ofast or -Os/-Oz.
> 

I see and fully agree. 
I'll find a better way to resolve the conflict, 
My current idea is to diasble zcmp when shrink-wrap-separate is actually active. 

Thanks Kito and Andrew Pinski for your patience.

BR, 
Fei
>On Mon, May 8, 2023 at 4:54 PM Fei Gao <gaofei@eswincomputing.com> wrote:
>>
>> On 2023-05-08 16:05  Kito Cheng <kito.cheng@gmail.com> wrote:
>> >
>> >> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> >> > index 45a63cab9c9..629e5e45cac 100644
>> >> > --- a/gcc/config/riscv/riscv.cc
>> >> > +++ b/gcc/config/riscv/riscv.cc
>> >> > @@ -5729,7 +5729,8 @@ riscv_get_separate_components (void)
>> >> >
>> >> >    if (riscv_use_save_libcall (&cfun->machine->frame)
>> >> >        || cfun->machine->interrupt_handler_p
>> >> > -      || !cfun->machine->frame.gp_sp_offset.is_constant ())
>> >> > +      || !cfun->machine->frame.gp_sp_offset.is_constant ()
>> >> > +      || TARGET_ZCMP)
>> >> >      return components;
>> >>
>> >> I think this is a bad idea. I have a use case where we use the C
>> >> extensions but still compile for -O2 because we want the code to be
>> >> fast as possible but still having the savings of the C extensions.
>> >
>> >Yeah, agree, so I would prefer to drop this from the patch series.
>>
>> Zcmp is a little different here than C.
>> C extension is done fully in AS.  So  we have the code to be
>> fast as possible but still having the savings of the C extensions.
>>
>> Zcmp and shrink-wrap-separate are both done in prologue/epilogue pass
>> and you can only have one switch active to direct sregs save and restore.
>> In my understanding, zcmp push and pop insns seem to
>> be mutually exclusive in functionality to shrink-wrap-separate.
>> It's not expected to see zcmp insns at the begining/end of prologue/epilogue,
>> and also repeated store/load sregs in separate blocks.
>>
>> Same for save and restore, and i guess that's why we have
>> riscv_use_save_libcall (&cfun->machine->frame) check here.
>>
>> BR,
>> Fei
>>
>> >
>> >> Thanks,
>> >> Andrew Pinski
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 45a63cab9c9..629e5e45cac 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -5729,7 +5729,8 @@  riscv_get_separate_components (void)
 
   if (riscv_use_save_libcall (&cfun->machine->frame)
       || cfun->machine->interrupt_handler_p
-      || !cfun->machine->frame.gp_sp_offset.is_constant ())
+      || !cfun->machine->frame.gp_sp_offset.is_constant ()
+      || TARGET_ZCMP)
     return components;
 
   offset = cfun->machine->frame.gp_sp_offset.to_constant ();