[1/2] xtensa: Fix non-fatal regression introduced by b2ef02e8cbbaf95fee98be255f697f47193960ec
Message ID | ecfe3ee6-f897-1939-05ea-3e427ad53ae6@yahoo.co.jp |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1968F385800A for <patchwork@sourceware.org>; Thu, 23 Feb 2023 03:43:28 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 1968F385800A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1677123808; bh=+SSbSPmy9H2/1/5Sc5AtDq2Jup2A3WROayHldUIwFTw=; h=Date:To:Cc:Subject:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=ex1jQ9NfpyRSEj9w3iczQfBxtKdDJIWy+bgpuY/jH1Lkg7/SUD2OAR5rTqsv6nMiE 0ZV6bEuf52yH+UJUymeLsIq+RFrIfvLKEHl+OlP9cg0A6mhdCtzYaSeg/bC0jxadf+ nZDI620kg7cbvb0bDlCNg5N3sfpY3MaQG3VI8l4o= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from sonicconh6001-vm2.mail.ssk.yahoo.co.jp (sonicconh6001-vm2.mail.ssk.yahoo.co.jp [182.22.37.11]) by sourceware.org (Postfix) with ESMTPS id 965AD3858D33 for <gcc-patches@gcc.gnu.org>; Thu, 23 Feb 2023 03:42:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 965AD3858D33 X-YMail-OSG: qDImPe8VM1kH6VHA.8BOW0N9DJTpFhqr1SCCi0.wa.ZTt5V4RdmOZoaxA.wRQeK fSWORuN680EA1zlBDi4XDSB8hxwMOe9tZNiuWEZKo1voR26vAZ2Z5ecCYNBwaC8juUh29nflUFeh QXX1bkQIJTyjXnjV4G04VVLEKA_SCjB9x42hlXaUWN65wNVVbuSycwjrVpd7pJRenXuBbcjfykMB v3XBEr130SFD3fcioLn_TQZxqVozQH.ROBrVVZJkVRUy9Xub22L_KSjBBo2wBtVg.TXHbkOpnbed _wdTc4BZAGoUrsuosZUrIxDf68v3VrBsaejz24Ej8jkROQYU4R_6noxgjOkNdxUVhAcmgYaAqbQF dyPKsVS4x7KTUTCVnkyxe_XiKh7rjMw9mYtrd3Ng6oHmET8Nml45Fu2geY..IQ26lB5UyqZCl4MG RTn9DE61MqStkMQgow6XGQs3ooTWycqluLbaG2Tg.buEer3_moo1h.5wnuJHuWDkc1qkTcDdGleM 8odKkX1vs1Eug32h1Y2QONBUO2BzpvQ2z5ycMqtExLvkFs0uC67VxntsgY92qUjYHcwNdSkWftZ6 kOobVbe65hxM0M6hIvVoeHRjkBlFRNmlo7c7EOja9U22W3tu4x.wS7ZpCM0GYgXx4EWtrQfZsyTq 9BTOtzUr9dMQnONO3wDYEkyRhv6lpPWMOxiMvxGWV4gQRk.rZaplIHh2_ibNq8CnVLBPBUzkJNk6 jCZc1FkLk0q.60NfAuNRJ5UnyTCMLII9dt6u9DhassYSq7ghf5VaLp4ySE7Up2p0J4DNkt4uZP9M lgY6w1UcUnip88BYy5wJYQqjQ_dZMTFmCAETdWXwe7ag2UktUHBiHsgxErQPOAHF.jF48mN1zjBD T.t3sal83KfynO03oHQgGj41e32jnADP.CNnkzc4qA0neGM7.hudiWGERGMgSpoGQRFG3lh1n4vc 78fc- Received: from sonicgw.mail.yahoo.co.jp by sonicconh6001.mail.ssk.yahoo.co.jp with HTTP; Thu, 23 Feb 2023 03:42:52 +0000 Received: by smtphe6008.mail.ssk.ynwp.yahoo.co.jp (YJ Hermes SMTP Server) with ESMTPA ID ec7ee38331a64c6d2ee42a6b1d9705ff; Thu, 23 Feb 2023 12:42:51 +0900 (JST) Message-ID: <ecfe3ee6-f897-1939-05ea-3e427ad53ae6@yahoo.co.jp> Date: Thu, 23 Feb 2023 12:41:40 +0900 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 To: GCC Patches <gcc-patches@gcc.gnu.org> Cc: Max Filippov <jcmvbkbc@gmail.com> Subject: [PATCH 1/2] xtensa: Fix non-fatal regression introduced by b2ef02e8cbbaf95fee98be255f697f47193960ec Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit References: <ecfe3ee6-f897-1939-05ea-3e427ad53ae6.ref@yahoo.co.jp> X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Takayuki 'January June' Suwa via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Takayuki 'January June' Suwa <jjsuwa_sys3175@yahoo.co.jp> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[1/2] xtensa: Fix non-fatal regression introduced by b2ef02e8cbbaf95fee98be255f697f47193960ec
|
|
Commit Message
Takayuki 'January June' Suwa
Feb. 23, 2023, 3:41 a.m. UTC
In commit b2ef02e8cbbaf95fee98be255f697f47193960ec, the sibling call
insn included (use (reg:SI A0_REG)) to fix the problem, which added
a USE chain unconditionally to the data flow of register A0 during
the sibling call.
As a result, df_regs_ever_live_p (A0_REG) returns true, so even if
register A0 is not used outside of the sibling call insn, saves and
restores to stack slots are emitted in pro/epilogue, and finally
code size increases.
(This is why I never included (use A0) in sibling calls)
/* example */
extern int foo(int);
int test(int a) {
return foo(a * 3 + 1);
}
;; before
test:
addi sp, sp, -16 ;; unneeded stack frame allocation (induced)
s32i.n a0, sp, 12 ;; unneeded saving of register A0
l32i.n a0, sp, 12 ;; unneeded restoration of register A0
addx2 a2, a2, a2
addi.n a2, a2, 1
addi sp, sp, 16 ;; unneeded stack frame freeing (induced)
j.l foo, a9 ;; sibling call (truly needs register A0)
The essential cause is that we emit (use A0) *before* the insns that
does the stack pointer adjustment during epilogue expansion, so the
liveness of register A0 ends early, so register A0 is reused afterwards.
This patch fixes the problem and avoids such regression by doing the
emit of (use A0) in the sibling call epilogue expansion at the end.
;; after
test:
addx2 a2, a2, a2
addi.n a2, a2, 1
j.l foo, a9
>From RTL-pass "315r.rnreg" by
"gfortran -O3 -funroll-loops -mabi=call0 -S -da gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":
;; Function selector_init (__selectors_MOD_selector_init, funcdef_no=2, decl_uid=987, cgraph_uid=3, symbol_order=4)
...
(insn 3807 3806 3808 121 (set (reg:SI 15 a15)
(mem/c:SI (plus:SI (reg/f:SI 1 sp)
(const_int 268 [0x10c])) [31 S4 A32])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 53 {movsi_internal}
(nil))
(insn 3808 3807 3809 121 (set (reg:SI 7 a7)
(const_int 288 [0x120])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 53 {movsi_internal}
(nil))
(insn 3809 3808 3810 121 (set (reg/f:SI 1 sp)
(plus:SI (reg/f:SI 1 sp)
(reg:SI 7 a7))) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 1 {addsi3}
(expr_list:REG_DEAD (reg:SI 9 a9)
(nil)))
(insn 3810 3809 721 121 (use (reg:SI 0 a0)) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 -1
(expr_list:REG_DEAD (reg:SI 0 a0)
(nil)))
(call_insn/j 721 3810 722 121 (call (mem:SI (symbol_ref:SI ("free") [flags 0x41] <function_decl 0x7f7c885f6000 __builtin_free>) [0 __builtin_free S4 A32])
(const_int 0 [0])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 discrim 1 106 {sibcall_internal}
(expr_list:REG_DEAD (reg:SI 2 a2)
(expr_list:REG_CALL_DECL (symbol_ref:SI ("free") [flags 0x41] <function_decl 0x7f7c885f6000 __builtin_free>)
(expr_list:REG_EH_REGION (const_int 0 [0])
(nil))))
(expr_list:SI (use (reg:SI 2 a2))
(nil)))
(IMHO the "rnreg" pass doesn't take REG_ALLOC_ORDER into account;
it just seems to allocate registers in fixed_regs index order,
which may have hurt register A0 that became allocatable in the recent
patch)
gcc/ChangeLog:
* config/xtensa/xtensa.cc (xtensa_expand_epilogue):
Emit (use (reg:SI A0_REG)) at the end in the sibling call
(i.e. the same place as (return) in the normal call).
* config/xtensa/xtensa.md
(sibcall, sibcall_internal, sibcall_value, sibcall_value_internal):
Revert changes by the previous patch.
---
gcc/config/xtensa/xtensa.cc | 4 +++-
gcc/config/xtensa/xtensa.md | 20 +++++++-------------
2 files changed, 10 insertions(+), 14 deletions(-)
Comments
On Wed, Feb 22, 2023 at 7:42 PM Takayuki 'January June' Suwa <jjsuwa_sys3175@yahoo.co.jp> wrote: > > In commit b2ef02e8cbbaf95fee98be255f697f47193960ec, the sibling call > insn included (use (reg:SI A0_REG)) to fix the problem, which added > a USE chain unconditionally to the data flow of register A0 during > the sibling call. > > As a result, df_regs_ever_live_p (A0_REG) returns true, so even if > register A0 is not used outside of the sibling call insn, saves and > restores to stack slots are emitted in pro/epilogue, and finally > code size increases. > (This is why I never included (use A0) in sibling calls) > > /* example */ > extern int foo(int); > int test(int a) { > return foo(a * 3 + 1); > } > > ;; before > test: > addi sp, sp, -16 ;; unneeded stack frame allocation (induced) > s32i.n a0, sp, 12 ;; unneeded saving of register A0 > l32i.n a0, sp, 12 ;; unneeded restoration of register A0 > addx2 a2, a2, a2 > addi.n a2, a2, 1 > addi sp, sp, 16 ;; unneeded stack frame freeing (induced) > j.l foo, a9 ;; sibling call (truly needs register A0) > > The essential cause is that we emit (use A0) *before* the insns that > does the stack pointer adjustment during epilogue expansion, so the > liveness of register A0 ends early, so register A0 is reused afterwards. > > This patch fixes the problem and avoids such regression by doing the > emit of (use A0) in the sibling call epilogue expansion at the end. > > ;; after > test: > addx2 a2, a2, a2 > addi.n a2, a2, 1 > j.l foo, a9 > > >From RTL-pass "315r.rnreg" by > "gfortran -O3 -funroll-loops -mabi=call0 -S -da gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90": > > ;; Function selector_init (__selectors_MOD_selector_init, funcdef_no=2, decl_uid=987, cgraph_uid=3, symbol_order=4) > ... > (insn 3807 3806 3808 121 (set (reg:SI 15 a15) > (mem/c:SI (plus:SI (reg/f:SI 1 sp) > (const_int 268 [0x10c])) [31 S4 A32])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 53 {movsi_internal} > (nil)) > (insn 3808 3807 3809 121 (set (reg:SI 7 a7) > (const_int 288 [0x120])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 53 {movsi_internal} > (nil)) > (insn 3809 3808 3810 121 (set (reg/f:SI 1 sp) > (plus:SI (reg/f:SI 1 sp) > (reg:SI 7 a7))) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 1 {addsi3} > (expr_list:REG_DEAD (reg:SI 9 a9) > (nil))) > (insn 3810 3809 721 121 (use (reg:SI 0 a0)) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 -1 > (expr_list:REG_DEAD (reg:SI 0 a0) > (nil))) > (call_insn/j 721 3810 722 121 (call (mem:SI (symbol_ref:SI ("free") [flags 0x41] <function_decl 0x7f7c885f6000 __builtin_free>) [0 __builtin_free S4 A32]) > (const_int 0 [0])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 discrim 1 106 {sibcall_internal} > (expr_list:REG_DEAD (reg:SI 2 a2) > (expr_list:REG_CALL_DECL (symbol_ref:SI ("free") [flags 0x41] <function_decl 0x7f7c885f6000 __builtin_free>) > (expr_list:REG_EH_REGION (const_int 0 [0]) > (nil)))) > (expr_list:SI (use (reg:SI 2 a2)) > (nil))) > > (IMHO the "rnreg" pass doesn't take REG_ALLOC_ORDER into account; > it just seems to allocate registers in fixed_regs index order, > which may have hurt register A0 that became allocatable in the recent > patch) > > gcc/ChangeLog: > > * config/xtensa/xtensa.cc (xtensa_expand_epilogue): > Emit (use (reg:SI A0_REG)) at the end in the sibling call > (i.e. the same place as (return) in the normal call). > * config/xtensa/xtensa.md > (sibcall, sibcall_internal, sibcall_value, sibcall_value_internal): > Revert changes by the previous patch. > --- > gcc/config/xtensa/xtensa.cc | 4 +++- > gcc/config/xtensa/xtensa.md | 20 +++++++------------- > 2 files changed, 10 insertions(+), 14 deletions(-) I've reverted my fix and committed this fix minus the revert.
On Thu, Feb 23, 2023 at 1:35 AM Max Filippov <jcmvbkbc@gmail.com> wrote: > > On Wed, Feb 22, 2023 at 7:42 PM Takayuki 'January June' Suwa > <jjsuwa_sys3175@yahoo.co.jp> wrote: > > > > In commit b2ef02e8cbbaf95fee98be255f697f47193960ec, the sibling call > > insn included (use (reg:SI A0_REG)) to fix the problem, which added > > a USE chain unconditionally to the data flow of register A0 during > > the sibling call. > > > > As a result, df_regs_ever_live_p (A0_REG) returns true, so even if > > register A0 is not used outside of the sibling call insn, saves and > > restores to stack slots are emitted in pro/epilogue, and finally > > code size increases. > > (This is why I never included (use A0) in sibling calls) > > > > /* example */ > > extern int foo(int); > > int test(int a) { > > return foo(a * 3 + 1); > > } > > > > ;; before > > test: > > addi sp, sp, -16 ;; unneeded stack frame allocation (induced) > > s32i.n a0, sp, 12 ;; unneeded saving of register A0 > > l32i.n a0, sp, 12 ;; unneeded restoration of register A0 > > addx2 a2, a2, a2 > > addi.n a2, a2, 1 > > addi sp, sp, 16 ;; unneeded stack frame freeing (induced) > > j.l foo, a9 ;; sibling call (truly needs register A0) > > > > The essential cause is that we emit (use A0) *before* the insns that > > does the stack pointer adjustment during epilogue expansion, so the > > liveness of register A0 ends early, so register A0 is reused afterwards. > > > > This patch fixes the problem and avoids such regression by doing the > > emit of (use A0) in the sibling call epilogue expansion at the end. > > > > ;; after > > test: > > addx2 a2, a2, a2 > > addi.n a2, a2, 1 > > j.l foo, a9 > > > > >From RTL-pass "315r.rnreg" by > > "gfortran -O3 -funroll-loops -mabi=call0 -S -da gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90": > > > > ;; Function selector_init (__selectors_MOD_selector_init, funcdef_no=2, decl_uid=987, cgraph_uid=3, symbol_order=4) > > ... > > (insn 3807 3806 3808 121 (set (reg:SI 15 a15) > > (mem/c:SI (plus:SI (reg/f:SI 1 sp) > > (const_int 268 [0x10c])) [31 S4 A32])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 53 {movsi_internal} > > (nil)) > > (insn 3808 3807 3809 121 (set (reg:SI 7 a7) > > (const_int 288 [0x120])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 53 {movsi_internal} > > (nil)) > > (insn 3809 3808 3810 121 (set (reg/f:SI 1 sp) > > (plus:SI (reg/f:SI 1 sp) > > (reg:SI 7 a7))) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 1 {addsi3} > > (expr_list:REG_DEAD (reg:SI 9 a9) > > (nil))) > > (insn 3810 3809 721 121 (use (reg:SI 0 a0)) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 -1 > > (expr_list:REG_DEAD (reg:SI 0 a0) > > (nil))) > > (call_insn/j 721 3810 722 121 (call (mem:SI (symbol_ref:SI ("free") [flags 0x41] <function_decl 0x7f7c885f6000 __builtin_free>) [0 __builtin_free S4 A32]) > > (const_int 0 [0])) "gcc-gnu/gcc/testsuite/gfortran.dg/allocate_with_source_5.f90":35:30 discrim 1 106 {sibcall_internal} > > (expr_list:REG_DEAD (reg:SI 2 a2) > > (expr_list:REG_CALL_DECL (symbol_ref:SI ("free") [flags 0x41] <function_decl 0x7f7c885f6000 __builtin_free>) > > (expr_list:REG_EH_REGION (const_int 0 [0]) > > (nil)))) > > (expr_list:SI (use (reg:SI 2 a2)) > > (nil))) > > > > (IMHO the "rnreg" pass doesn't take REG_ALLOC_ORDER into account; > > it just seems to allocate registers in fixed_regs index order, > > which may have hurt register A0 that became allocatable in the recent > > patch) > > > > gcc/ChangeLog: > > > > * config/xtensa/xtensa.cc (xtensa_expand_epilogue): > > Emit (use (reg:SI A0_REG)) at the end in the sibling call > > (i.e. the same place as (return) in the normal call). > > * config/xtensa/xtensa.md > > (sibcall, sibcall_internal, sibcall_value, sibcall_value_internal): > > Revert changes by the previous patch. > > --- > > gcc/config/xtensa/xtensa.cc | 4 +++- > > gcc/config/xtensa/xtensa.md | 20 +++++++------------- > > 2 files changed, 10 insertions(+), 14 deletions(-) > > I've reverted my fix and committed this fix minus the revert. Sorry, I've messed up the patch authorship in the rebase ):
diff --git a/gcc/config/xtensa/xtensa.cc b/gcc/config/xtensa/xtensa.cc index 5c1c713e122..b80eef5c19e 100644 --- a/gcc/config/xtensa/xtensa.cc +++ b/gcc/config/xtensa/xtensa.cc @@ -3573,7 +3573,9 @@ xtensa_expand_epilogue (bool sibcall_p) EH_RETURN_STACKADJ_RTX)); } cfun->machine->epilogue_done = true; - if (!sibcall_p) + if (sibcall_p) + emit_use (gen_rtx_REG (SImode, A0_REG)); + else emit_jump_insn (gen_return ()); } diff --git a/gcc/config/xtensa/xtensa.md b/gcc/config/xtensa/xtensa.md index b8a8aaf9764..d3996b26cb5 100644 --- a/gcc/config/xtensa/xtensa.md +++ b/gcc/config/xtensa/xtensa.md @@ -2369,10 +2369,8 @@ (set_attr "length" "3")]) (define_expand "sibcall" - [(parallel [ - (call (match_operand 0 "memory_operand" "") - (match_operand 1 "" "")) - (use (reg:SI A0_REG))])] + [(call (match_operand 0 "memory_operand" "") + (match_operand 1 "" ""))] "!TARGET_WINDOWED_ABI" { xtensa_prepare_expand_call (0, operands); @@ -2380,8 +2378,7 @@ (define_insn "sibcall_internal" [(call (mem:SI (match_operand:SI 0 "call_insn_operand" "nic")) - (match_operand 1 "" "i")) - (use (reg:SI A0_REG))] + (match_operand 1 "" "i"))] "!TARGET_WINDOWED_ABI && SIBLING_CALL_P (insn)" { return xtensa_emit_sibcall (0, operands); @@ -2391,11 +2388,9 @@ (set_attr "length" "3")]) (define_expand "sibcall_value" - [(parallel [ - (set (match_operand 0 "register_operand" "") - (call (match_operand 1 "memory_operand" "") - (match_operand 2 "" ""))) - (use (reg:SI A0_REG))])] + [(set (match_operand 0 "register_operand" "") + (call (match_operand 1 "memory_operand" "") + (match_operand 2 "" "")))] "!TARGET_WINDOWED_ABI" { xtensa_prepare_expand_call (1, operands); @@ -2404,8 +2399,7 @@ (define_insn "sibcall_value_internal" [(set (match_operand 0 "register_operand" "=a") (call (mem:SI (match_operand:SI 1 "call_insn_operand" "nic")) - (match_operand 2 "" "i"))) - (use (reg:SI A0_REG))] + (match_operand 2 "" "i")))] "!TARGET_WINDOWED_ABI && SIBLING_CALL_P (insn)" { return xtensa_emit_sibcall (1, operands);