RE: [PATCH] PR rtl-optimization/110587: Reduce useless moves in compile-time hog.

["Roger Sayle" <roger@nextmovesoftware.com>]

[-- Attachment #1: Type: text/plain, Size: 4273 bytes --]


Hi Richard,

You're 100% right.  It’s possible to significantly clean-up this code, replacing
the body of the conditional with a call to force_reg and simplifying the conditions
under which it is called.  These improvements are implemented in the patch
below, which has been tested on x86_64-pc-linux-gnu, with a bootstrap and
make -k check, both with and without -m32, as usual.

Interestingly, the CONCAT clause afterwards is still required (I've learned something
new),  as calling force_reg (or gen_reg_rtx) with HCmode, actually returns a CONCAT
instead of a REG, so although the code looks dead, it's required to build libgcc during
a bootstrap.  But the remaining clean-up is good, reducing the number of source lines
and making the logic easier to understand.

Ok for mainline?

2023-07-27  Roger Sayle  <roger@nextmovesoftware.com>
            Richard Biener  <rguenther@suse.de>

gcc/ChangeLog
        PR middle-end/28071
        PR rtl-optimization/110587
        * expr.cc (emit_group_load_1): Simplify logic for calling
        force_reg on ORIG_SRC, to avoid making a copy if the source
        is already in a pseudo register.

Roger
--

> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: 25 July 2023 12:50
> 
> On Tue, Jul 25, 2023 at 1:31 PM Roger Sayle <roger@nextmovesoftware.com>
> wrote:
> >
> > This patch is the third in series of fixes for PR
> > rtl-optimization/110587, a compile-time regression with -O0, that
> > attempts to address the underlying cause.  As noted previously, the
> > pathological test case pr28071.c contains a large number of useless
> > register-to-register moves that can produce quadratic behaviour (in
> > LRA).  These move are generated during RTL expansion in
> > emit_group_load_1, where the middle-end attempts to simplify the
> > source before calling extract_bit_field.  This is reasonable if the
> > source is a complex expression (from before the tree-ssa optimizers),
> > or a SUBREG, or a hard register, but it's not particularly useful to
> > copy a pseudo register into a new pseudo register.  This patch eliminates that
> redundancy.
> >
> > The -fdump-tree-expand for pr28071.c compiled with -O0 currently
> > contains 777K lines, with this patch it contains 717K lines, i.e.
> > saving about 60K lines (admittedly of debugging text output, but it makes the
> point).
> >
> >
> > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> > and make -k check, both with and without --target_board=unix{-m32}
> > with no new failures.  Ok for mainline?
> >
> > As always, I'm happy to revert this change quickly if there's a
> > problem, and investigate why this additional copy might (still) be
> > needed on other
> > non-x86 targets.
> 
> @@ -2622,6 +2622,7 @@ emit_group_load_1 (rtx *tmps, rtx dst, rtx orig_src,
> tree type,
>          be loaded directly into the destination.  */
>        src = orig_src;
>        if (!MEM_P (orig_src)
> +         && (!REG_P (orig_src) || HARD_REGISTER_P (orig_src))
>           && (!CONSTANT_P (orig_src)
>               || (GET_MODE (orig_src) != mode
>                   && GET_MODE (orig_src) != VOIDmode)))
> 
> so that means the code guarded by the conditional could instead be transformed
> to
> 
>    src = force_reg (mode, orig_src);
> 
> ?  Btw, the || (GET_MODE (orig_src) != mode && GET_MODE (orig_src) !=
> VOIDmode) case looks odd as in that case we'd use GET_MODE (orig_src) for the
> move ... that might also mean we have to use force_reg (GET_MODE (orig_src) ==
> VOIDmode ? mode : GET_MODE (orig_src), orig_src))
> 
> Otherwise I think this is OK, as said, using force_reg somehow would improve
> readability here I think.
> 
> I also wonder how the
> 
>       else if (GET_CODE (src) == CONCAT)
> 
> case will ever trigger with the current code.
> 
> Richard.
> 
> >
> > 2023-07-25  Roger Sayle  <roger@nextmovesoftware.com>
> >
> > gcc/ChangeLog
> >         PR middle-end/28071
> >         PR rtl-optimization/110587
> >         * expr.cc (emit_group_load_1): Avoid copying a pseudo register into
> >         a new pseudo register, i.e. only copy hard regs into a new pseudo.
> >
> >


[-- Attachment #2: patchhg4.txt --]
[-- Type: text/plain, Size: 846 bytes --]

diff --git a/gcc/expr.cc b/gcc/expr.cc
index fff09dc..174f8ac 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -2622,16 +2622,11 @@ emit_group_load_1 (rtx *tmps, rtx dst, rtx orig_src, tree type,
 	 be loaded directly into the destination.  */
       src = orig_src;
       if (!MEM_P (orig_src)
-	  && (!CONSTANT_P (orig_src)
-	      || (GET_MODE (orig_src) != mode
-		  && GET_MODE (orig_src) != VOIDmode)))
+	  && (!REG_P (orig_src) || HARD_REGISTER_P (orig_src))
+	  && !CONSTANT_P (orig_src))
 	{
-	  if (GET_MODE (orig_src) == VOIDmode)
-	    src = gen_reg_rtx (mode);
-	  else
-	    src = gen_reg_rtx (GET_MODE (orig_src));
-
-	  emit_move_insn (src, orig_src);
+	  gcc_assert (GET_MODE (orig_src) != VOIDmode);
+	  src = force_reg (GET_MODE (orig_src), orig_src);
 	}
 
       /* Optimize the access just a bit.  */