public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [COMMITTED 1/2] Revert "gcc: xtensa: fix PR target/108876"
@ 2023-02-23  9:37 Max Filippov
  2023-02-23  9:37 ` [COMMITTED 2/2] xtensa: fix PR target/108876 Max Filippov
  0 siblings, 1 reply; 2+ messages in thread
From: Max Filippov @ 2023-02-23  9:37 UTC (permalink / raw)
  To: gcc-patches; +Cc: Takayuki 'January June' Suwa, Max Filippov

This reverts commit b2ef02e8cbbaf95fee98be255f697f47193960ec.
---
 gcc/config/xtensa/xtensa.cc |  2 ++
 gcc/config/xtensa/xtensa.md | 20 +++++++-------------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/gcc/config/xtensa/xtensa.cc b/gcc/config/xtensa/xtensa.cc
index 5c1c713e122d..d0320efe21d4 100644
--- a/gcc/config/xtensa/xtensa.cc
+++ b/gcc/config/xtensa/xtensa.cc
@@ -3548,6 +3548,8 @@ xtensa_expand_epilogue (bool sibcall_p)
 			      gen_frame_mem (SImode, x));
 	    }
 	}
+      if (sibcall_p)
+	emit_use (gen_rtx_REG (SImode, A0_REG));
 
       if (cfun->machine->current_frame_size > 0)
 	{
diff --git a/gcc/config/xtensa/xtensa.md b/gcc/config/xtensa/xtensa.md
index b8a8aaf97640..d3996b26cb5c 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);
-- 
2.30.2


^ permalink raw reply	[flat|nested] 2+ messages in thread

* [COMMITTED 2/2] xtensa: fix PR target/108876
  2023-02-23  9:37 [COMMITTED 1/2] Revert "gcc: xtensa: fix PR target/108876" Max Filippov
@ 2023-02-23  9:37 ` Max Filippov
  0 siblings, 0 replies; 2+ messages in thread
From: Max Filippov @ 2023-02-23  9:37 UTC (permalink / raw)
  To: gcc-patches; +Cc: Takayuki 'January June' Suwa, Max Filippov

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:
	PR target/108876

	* 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).
---
 gcc/config/xtensa/xtensa.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/config/xtensa/xtensa.cc b/gcc/config/xtensa/xtensa.cc
index d0320efe21d4..b80eef5c19ef 100644
--- a/gcc/config/xtensa/xtensa.cc
+++ b/gcc/config/xtensa/xtensa.cc
@@ -3548,8 +3548,6 @@ xtensa_expand_epilogue (bool sibcall_p)
 			      gen_frame_mem (SImode, x));
 	    }
 	}
-      if (sibcall_p)
-	emit_use (gen_rtx_REG (SImode, A0_REG));
 
       if (cfun->machine->current_frame_size > 0)
 	{
@@ -3575,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 ());
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2023-02-23  9:37 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-23  9:37 [COMMITTED 1/2] Revert "gcc: xtensa: fix PR target/108876" Max Filippov
2023-02-23  9:37 ` [COMMITTED 2/2] xtensa: fix PR target/108876 Max Filippov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).