Re: [PATCH] i386: Fix up replacement of registers in certain peephole2s [PR108832]

[Uros Bizjak <ubizjak@gmail.com>]

On Sat, Feb 18, 2023 at 11:35 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> As mentioned in the PR, replace_rtx has 2 modes, one that only replaces
> x == from with to, the other which i386.md uses which also replaces
> REGNO (x) == REGNO (from) with to if both are REGs, but assert they have
> the same mode.  This is reasonable behavior if one replaces from with
> some other expression, say constant etc., but ICEs whenever the register
> appears in a different mode, which happens e.g. on the following testcase,
> where from/to has DImode but inside of the operands we have SImode of
> the from register.  replace_rtx also does some limited simplifications
> (though far less than simplify_replace_fn_rtx), which is needed if
> from a REG is replaced say with CONST_INT, but the peephole2s that use
> this only replace one REG with another one.
>
> The following patch introduces a new backend function for this, avoids doing
> any simplifications and just replaces the REGs, for safety on a copy of
> the expression if any changes will be needed.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2023-02-18  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/108832
>         * config/i386/i386-protos.h (ix86_replace_reg_with_reg): Declare.
>         * config/i386/i386-expand.cc (ix86_replace_reg_with_reg): New
>         function.
>         * config/i386/i386.md: Replace replace_rtx calls in all peephole2s
>         with ix86_replace_reg_with_reg.
>
>         * gcc.target/i386/pr108832.c: New test.

OK.

Thanks,
Uros.

>
> --- gcc/config/i386/i386-protos.h.jj    2023-01-16 11:52:15.960735877 +0100
> +++ gcc/config/i386/i386-protos.h       2023-02-17 14:32:27.426259498 +0100
> @@ -168,6 +168,7 @@ extern void ix86_split_lshr (rtx *, rtx,
>  extern void ix86_expand_v1ti_shift (enum rtx_code, rtx[]);
>  extern void ix86_expand_v1ti_rotate (enum rtx_code, rtx[]);
>  extern void ix86_expand_v1ti_ashiftrt (rtx[]);
> +extern rtx ix86_replace_reg_with_reg (rtx, rtx, rtx);
>  extern rtx ix86_find_base_term (rtx);
>  extern bool ix86_check_movabs (rtx, int);
>  extern bool ix86_check_no_addr_space (rtx);
> --- gcc/config/i386/i386-expand.cc.jj   2023-01-31 10:12:12.636168637 +0100
> +++ gcc/config/i386/i386-expand.cc      2023-02-17 14:32:56.462839237 +0100
> @@ -7093,6 +7093,37 @@ ix86_expand_v1ti_ashiftrt (rtx operands[
>      }
>  }
>
> +/* Replace all occurrences of REG FROM with REG TO in X, including
> +   occurrences with different modes.  */
> +
> +rtx
> +ix86_replace_reg_with_reg (rtx x, rtx from, rtx to)
> +{
> +  gcc_checking_assert (REG_P (from)
> +                      && REG_P (to)
> +                      && GET_MODE (from) == GET_MODE (to));
> +  if (!reg_overlap_mentioned_p (from, x))
> +    return x;
> +  rtx ret = copy_rtx (x);
> +  subrtx_ptr_iterator::array_type array;
> +  FOR_EACH_SUBRTX_PTR (iter, array, &ret, NONCONST)
> +    {
> +      rtx *loc = *iter;
> +      x = *loc;
> +      if (REG_P (x) && REGNO (x) == REGNO (from))
> +       {
> +         if (x == from)
> +           *loc = to;
> +         else
> +           {
> +             gcc_checking_assert (REG_NREGS (x) == 1);
> +             *loc = gen_rtx_REG (GET_MODE (x), REGNO (to));
> +           }
> +       }
> +    }
> +  return ret;
> +}
> +
>  /* Return mode for the memcpy/memset loop counter.  Prefer SImode over
>     DImode for constant loop counts.  */
>
> --- gcc/config/i386/i386.md.jj  2023-02-16 10:13:23.705210608 +0100
> +++ gcc/config/i386/i386.md     2023-02-17 14:34:22.606592399 +0100
> @@ -22037,8 +22037,10 @@ (define_peephole2
>                                           (match_dup 0)))]
>  {
>    operands[7] = SET_DEST (XVECEXP (PATTERN (peep2_next_insn (1)), 0, 0));
> -  operands[8] = replace_rtx (operands[5], operands[0], operands[1], true);
> -  operands[9] = replace_rtx (operands[6], operands[0], operands[1], true);
> +  operands[8]
> +    = ix86_replace_reg_with_reg (operands[5], operands[0], operands[1]);
> +  operands[9]
> +    = ix86_replace_reg_with_reg (operands[6], operands[0], operands[1]);
>  })
>
>  ;; Eliminate a reg-reg mov by inverting the condition of a cmov (#2).
> @@ -22070,8 +22072,10 @@ (define_peephole2
>                                           (match_dup 0)))]
>  {
>    operands[7] = SET_DEST (XVECEXP (PATTERN (peep2_next_insn (2)), 0, 0));
> -  operands[8] = replace_rtx (operands[5], operands[0], operands[1], true);
> -  operands[9] = replace_rtx (operands[6], operands[0], operands[1], true);
> +  operands[8]
> +    = ix86_replace_reg_with_reg (operands[5], operands[0], operands[1]);
> +  operands[9]
> +    = ix86_replace_reg_with_reg (operands[6], operands[0], operands[1]);
>  })
>
>  (define_insn "movhf_mask"
> @@ -23210,7 +23214,10 @@ (define_peephole2
>     (parallel [(set (match_dup 0)
>                    (match_op_dup 3 [(match_dup 0) (match_dup 1)]))
>               (clobber (reg:CC FLAGS_REG))])]
> -  "operands[4] = replace_rtx (operands[2], operands[0], operands[1], true);")
> +{
> +  operands[4]
> +    = ix86_replace_reg_with_reg (operands[2], operands[0], operands[1]);
> +})
>
>  (define_peephole2
>    [(set (match_operand 0 "mmx_reg_operand")
> --- gcc/testsuite/gcc.target/i386/pr108832.c.jj 2023-02-17 14:40:39.568136274 +0100
> +++ gcc/testsuite/gcc.target/i386/pr108832.c    2023-02-17 14:39:36.846044106 +0100
> @@ -0,0 +1,19 @@
> +/* PR target/108832 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -funroll-loops" } */
> +
> +unsigned int m;
> +short int n;
> +
> +long int
> +bar (unsigned int x)
> +{
> +  return x ? x : 1;
> +}
> +
> +__attribute__ ((simd)) void
> +foo (void)
> +{
> +  int a = m / bar (3);
> +  n = 1 % bar (a << 1);
> +}
>
>         Jakub
>