diff mbox series

xtensa: Prevent emitting integer additions of constant zero

Message ID b67a8fb6-1a83-2806-7472-3f115f302a80@yahoo.co.jp
State New
Headers
Series xtensa: Prevent emitting integer additions of constant zero |

Commit Message

Takayuki 'January June' Suwa Aug. 16, 2022, 12:40 p.m. UTC 
  In a few cases, obviously omitable add instructions can be emitted via
invoking gen_addsi3.

gcc/ChangeLog:

	* config/xtensa/xtensa.md (addsi3_internal): Rename from "addsi3".
	(addsi3): New define_expand in order to reject integer additions of
	constant zero.
---
 gcc/config/xtensa/xtensa.md | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Max Filippov Aug. 16, 2022, 7:58 p.m. UTC | #1 
Hi Suwa-san,

On Tue, Aug 16, 2022 at 5:42 AM Takayuki 'January June' Suwa
<jjsuwa_sys3175@yahoo.co.jp> wrote:
>
> In a few cases, obviously omitable add instructions can be emitted via
> invoking gen_addsi3.
>
> gcc/ChangeLog:
>
>         * config/xtensa/xtensa.md (addsi3_internal): Rename from "addsi3".
>         (addsi3): New define_expand in order to reject integer additions of
>         constant zero.
> ---
>  gcc/config/xtensa/xtensa.md | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

with this change a bunch of tests fail to build with the following error:
  undefined reference to `__addsi3'

E.g. gcc.c-torture/execute/20000519-1.c
or gcc.c-torture/execute/20070919-1.c
Takayuki 'January June' Suwa Aug. 17, 2022, 9:51 a.m. UTC | #2 
On 2022/08/17 4:58, Max Filippov wrote:
> Hi Suwa-san,
Hi!

> 
> On Tue, Aug 16, 2022 at 5:42 AM Takayuki 'January June' Suwa
> <jjsuwa_sys3175@yahoo.co.jp> wrote:
>>
>> In a few cases, obviously omitable add instructions can be emitted via
>> invoking gen_addsi3.
>>
>> gcc/ChangeLog:
>>
>>         * config/xtensa/xtensa.md (addsi3_internal): Rename from "addsi3".
>>         (addsi3): New define_expand in order to reject integer additions of
>>         constant zero.
>> ---
>>  gcc/config/xtensa/xtensa.md | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> with this change a bunch of tests fail to build with the following error:

Ah, sorry, I want to withdraw this patch.

>> In a few cases

As a matter of fact, "in a few cases" is just only one:

[xtensa_expand_epilogue() in /gcc/config/xtensa/xtensa.cc]
>      if (cfun->machine->current_frame_size > 0)
>	{
>	  if (frame_pointer_needed || /* always reachable with addi */
>	      cfun->machine->current_frame_size > 1024 ||
>	      cfun->machine->current_frame_size <= 127)
>	    {
>	      if (cfun->machine->current_frame_size <= 127)
>		offset = cfun->machine->current_frame_size;
>	      else
>		offset = cfun->machine->callee_save_size;
>
>	      emit_insn (gen_addsi3 (stack_pointer_rtx,
>				     stack_pointer_rtx,
>				     GEN_INT (offset)));	// offset can be zero!
>	    }

And adding "define_expand" only to deal with one rare case had too much impact, as you saw...

>   undefined reference to `__addsi3'
> 
> E.g. gcc.c-torture/execute/20000519-1.c
> or gcc.c-torture/execute/20070919-1.c
>
Max Filippov Aug. 17, 2022, 6:43 p.m. UTC | #3 
On Wed, Aug 17, 2022 at 2:52 AM Takayuki 'January June' Suwa
<jjsuwa_sys3175@yahoo.co.jp> wrote:
> As a matter of fact, "in a few cases" is just only one:
>
> [xtensa_expand_epilogue() in /gcc/config/xtensa/xtensa.cc]
> >      if (cfun->machine->current_frame_size > 0)
> >       {
> >         if (frame_pointer_needed || /* always reachable with addi */
> >             cfun->machine->current_frame_size > 1024 ||
> >             cfun->machine->current_frame_size <= 127)
> >           {
> >             if (cfun->machine->current_frame_size <= 127)
> >               offset = cfun->machine->current_frame_size;
> >             else
> >               offset = cfun->machine->callee_save_size;
> >
> >             emit_insn (gen_addsi3 (stack_pointer_rtx,
> >                                    stack_pointer_rtx,
> >                                    GEN_INT (offset)));        // offset can be zero!
> >           }

Oh, nice catch! I'll post a patch that adds a check for non-zero offset here.
diff mbox series

Patch

diff --git a/gcc/config/xtensa/xtensa.md b/gcc/config/xtensa/xtensa.md
index 9eeb73915..c132c1626 100644
--- a/gcc/config/xtensa/xtensa.md
+++ b/gcc/config/xtensa/xtensa.md
@@ -156,7 +156,19 @@ 
 
 ;; Addition.
 
-(define_insn "addsi3"
+(define_expand "addsi3"
+  [(set (match_operand:SI 0 "register_operand")
+	(plus:SI (match_operand:SI 1 "register_operand")
+		 (match_operand:SI 2 "add_operand")))]
+  ""
+{
+  if (! CONST_INT_P (operands[2]) || INTVAL (operands[2]) != 0)
+    emit_insn (gen_addsi3_internal (operands[0],
+				    operands[1], operands[2]));
+  DONE;
+})
+
+(define_insn "addsi3_internal"
   [(set (match_operand:SI 0 "register_operand" "=D,D,a,a,a")
 	(plus:SI (match_operand:SI 1 "register_operand" "%d,d,r,r,r")
 		 (match_operand:SI 2 "add_operand" "d,O,r,J,N")))]