diff mbox series

[SH,committed] : Fix outage caused by secondary combine pass (was: Re: [RFC/PATCH] libgcc: sh: Use soft-fp for non-hosted SH3/SH4)

Message ID e6ea5a02e4c7c0d2b9217849d125a580cf67af0a.camel@gmail.com
State New
Headers show
Series [SH,committed] : Fix outage caused by secondary combine pass (was: Re: [RFC/PATCH] libgcc: sh: Use soft-fp for non-hosted SH3/SH4) | expand

Commit Message

Oleg Endo July 21, 2024, 5:33 a.m. UTC 
Hi,

I've committed the attached patch to fix the full gcc + libstdc++ build on
sh-elf.

Best regards,
Oleg Endo



On Sat, 2024-07-06 at 07:35 -0600, Jeff Law wrote:
> 
> On 7/5/24 1:28 AM, Sébastien Michelland wrote:
> > Hi Oleg!
> > 
> > > I don't understand why this is being limited to SH3 and SH4 only?
> > > Almost all SH4 systems out there have an FPU (unless special 
> > > configurations
> > > are used).  So I'd say if switching to soft-fp, then for SH-anything, not
> > > just SH3/SH4.
> > > 
> > > If it yields some improvements for some users, I'm all for it.
> > 
> > Yeah I just defaulted to SH3/SH4 conservatively because that's the only 
> > hardware I have. (My main platform also happens to be one of these SH4 
> > without an FPU, the SH4AL-DSP.)
> > 
> > Once this is tested/validated on simulator, I'll happily simplify the 
> > patch to apply to all SH.
> > 
> > > I think it would make sense to test it using sh-sim on SH2 big-endian and
> > > little endian at least, as that doesn't have an FPU and hence would run
> > > tests utilizing soft-fp.
> > > 
> > > After building the toolchain for --target=sh-elf, you can use this to run
> > > the testsuite in the simulator:
> > > 
> > > make -k check RUNTESTFLAGS="--target_board=sh-sim\{-m2/-ml,-m2/-mb}"
> > > 
> > > (add make -j parameter according to you needs -- it will be slow)
> > 
> > Alright, it might take a little bit.
> > 
> > Building the combined tree of gcc/binutils/newlib masters (again 
> > following [1]) gives me an ICE in libstdc++v3/src/libbacktrace, 
> > irrespective of my libgcc change:
> This is almost certainly a poorly written pattern.  I just fixed a bunch 
> of these, but not this one.  Essentially a recent change in the generic 
> parts of the compiler is exposing some bugs in the SH backend. 
> Specifically:
> 
> > ;; Store (negated) T bit as all zeros or ones in a reg.  
> > ;;      subc    Rn,Rn   ! Rn = Rn - Rn - T; T = T
> > ;;      not     Rn,Rn   ! Rn = 0 - Rn
> > ;; 
> > ;; Note the call to sh_split_treg_set_expr may clobber
> > ;; the T reg.  We must express this, even though it's
> > ;; not immediately obvious this pattern changes the
> > ;; T register.
> > (define_insn_and_split "mov_neg_si_t"
> >   [(set (match_operand:SI 0 "arith_reg_dest" "=r")
> >         (neg:SI (match_operand 1 "treg_set_expr")))
> >    (clobber (reg:SI T_REG))] 
> >   "TARGET_SH1" 
> > {
> >   gcc_assert (t_reg_operand (operands[1], VOIDmode));
> >   return "subc  %0,%0";
> > }
> >   "&& can_create_pseudo_p () && !t_reg_operand (operands[1], VOIDmode)"
> >   [(const_int 0)]
> > {
> >   sh_treg_insns ti = sh_split_treg_set_expr (operands[1], curr_insn);
> >   emit_insn (gen_mov_neg_si_t (operands[0], get_t_reg_rtx ()));
> > 
> >   if (ti.remove_trailing_nott ())
> >     emit_insn (gen_one_cmplsi2 (operands[0], operands[0]));
> > 
> >   DONE; 
> > }
> >   [(set_attr "type" "arith")])
> 
> 
> As written this pattern could match after register allocation is 
> complete and thus we can't create new pseudos (the condition TARGET_SH1 
> controls that behavior).  operands[1] won't necessarily be the T 
> register in that case.
> 
> The split condition fails because we can't create new pseudos, so it's 
> left as-is.  At final assembly time the assertion triggers.
> 
> the "&& can_create_pseudo ()" part of the split condition should be 
> moved into the main condition.  I think that's all that's necessary to 
> fix this problem.  It'd probably be best of Oleg went through the 
> various define_insn_and_split patterns that utilize can_create_pseudo in 
> their split condition and evaluated them.
> 
> I only fixed the most obvious cases in my change from this morning.  I 
> don't typically work on the SH port and for changes which aren't 
> obviously correct, Oleg is in a better position to evaluate the proper fix.
> 
> jeff
diff mbox series

Patch

From 9e740e7d71d02369774e1380902bddd9681c463f Mon Sep 17 00:00:00 2001
From: Oleg Endo <olegendo@gcc.gnu.org>
Date: Sun, 21 Jul 2024 14:11:21 +0900
Subject: [PATCH] SH: Fix outage caused by recently added 2nd combine pass after reg alloc

I've also confirmed on the CSiBE set that the secondary combine pass is
actually beneficial on SH.  It does result in some code size reductions.

gcc/CHangeLog:
	* config/sh/sh.md (mov_neg_si_t): Allow insn and split after
	register allocation.
	(*treg_noop_move): New insn.
---
 gcc/config/sh/sh.md | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
index 3e97825..7eee12c 100644
--- a/gcc/config/sh/sh.md
+++ b/gcc/config/sh/sh.md
@@ -8407,21 +8407,29 @@ 
 {
   gcc_assert (t_reg_operand (operands[1], VOIDmode));
   return "subc	%0,%0";
 }
-  "&& can_create_pseudo_p () && !t_reg_operand (operands[1], VOIDmode)"
+  "&& !t_reg_operand (operands[1], VOIDmode)"
   [(const_int 0)]
 {
   sh_treg_insns ti = sh_split_treg_set_expr (operands[1], curr_insn);
   emit_insn (gen_mov_neg_si_t (operands[0], get_t_reg_rtx ()));
 
   if (ti.remove_trailing_nott ())
     emit_insn (gen_one_cmplsi2 (operands[0], operands[0]));
 
   DONE;
 }
   [(set_attr "type" "arith")])
 
+;; no-op T bit move which can result from other optimizations.
+(define_insn_and_split "*treg_noop_move"
+  [(set (reg:SI T_REG) (reg:SI T_REG))]
+  "TARGET_SH1"
+  "#"
+  "&& 1"
+  [(const_int 0)])
+
 ;; Invert the T bit.
 ;; On SH2A we can use the nott insn.  On anything else this must be done with
 ;; multiple insns like:
 ;;	movt	Rn
--
libgit2 1.7.2