| Message ID | b67a8fb6-1a83-2806-7472-3f115f302a80@yahoo.co.jp |
|---|---|
| State | New |
| Headers | show |
| Series | xtensa: Prevent emitting integer additions of constant zero | expand |
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
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 >
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 --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")))]