Re: PR 60604: CANNOT_CHANGE_MODE_CLASS being ignored

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Mar 31, 2014 at 6:42 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> PR 60604 shows a case where CANNOT_CHANGE_MODE_CLASS is being ignored
> for a subreg of a floating-point register, causing it to be replaced
> with an invalid (reg ...).
>
> There are various reasons why MIPS floating-point registers can't change
> mode, but one important one for big-endian 32-bit targets is that the
> registers are always little-endian.  This means an SImode subreg of
> a DFmode value refers to the opposite half from what GCC expects.
> (This could be fixed by renumbering the registers internally, but since
> the FPRs can't change mode for other reasons, it doesn't really seem
> worth it.)
>
> Before IRA we have:
>
>    (set (reg:DF 44 $f12)
>         (reg/v:DF 205 [ x ]))
>    (set (subreg:SI (reg:DF 199 [ D.3267 ]) 0)
>         (and:SI (subreg:SI (reg/v:DF 205 [ x ]) 0)
>                 (const_int 2147483647 [0x7fffffff])))
>    (set (subreg:SI (reg:DF 199 [ D.3267 ]) 4)
>         (subreg:SI (reg/v:DF 205 [ x ]) 4))
>
> IRA changes this to:
>
>    (set (reg:DF 44 $f12)
>         (reg/v:DF 205 [ x ]))
>    (set (subreg:SI (reg:DF 199 [ D.3267 ]) 0)
>         (and:SI (subreg:SI (reg:DF 44 $f12) 0)         <----
>                 (const_int 2147483647 [0x7fffffff])))
>    (set (subreg:SI (reg:DF 199 [ D.3267 ]) 4)
>         (subreg:SI (reg:DF 44 $f12) 4))                <----
>
> And reload creates the following reloads:
>
> Reload 0: reload_in (SI) = (reg:SI 44 $f12)
>         GR_REGS, RELOAD_FOR_INPUT (opnum = 1), can't combine
>         reload_in_reg: (subreg:SI (reg:DF 44 $f12) 0)
>         reload_reg_rtx: (reg:SI 2 $2)
> ...
> Reload 0: reload_in (DF) = (reg:DF 44 $f12)
>         GR_REGS, RELOAD_FOR_INPUT (opnum = 1)
>         reload_in_reg: (reg:DF 44 $f12)
>         reload_reg_rtx: (reg:DF 2 $2)
>
> The second entry, for the lowpart (subreg:SI (reg:DF 44 $f12) 4),
> correctly reloads the full $f12 value into a GPR.  But the first entry,
> for the highpart (subreg:SI (reg:DF 44 $f12) 0), gets simplified to
> (reg:SI $f12) in spite of CANNOT_CHANGE_MODE_CLASS.
>
> The difference comes from the fact that push_reload handles lowpart
> subregs differently from others.  The lowpart code is:
>
>   if (in != 0 && GET_CODE (in) == SUBREG
>       && (subreg_lowpart_p (in) || strict_low)
>       && ([...gruesome condition...]
> #ifdef CANNOT_CHANGE_MODE_CLASS
>           || (REG_P (SUBREG_REG (in))
>               && REGNO (SUBREG_REG (in)) < FIRST_PSEUDO_REGISTER
>               && REG_CANNOT_CHANGE_MODE_P
>               (REGNO (SUBREG_REG (in)), GET_MODE (SUBREG_REG (in)), inmode))
> #endif
>           ))
>     {
>       ...
>     }
>
> with others being handled by:
>
>   if (in != 0 && reload_inner_reg_of_subreg (in, inmode, false))
>     {
>       ...
>     }
>
> where reload_inner_reg_of_subreg doesn't check CANNOT_CHANGE_MODE_CLASS.
> Simply adding:
>
> #ifdef CANNOT_CHANGE_MODE_CLASS
>   if (REG_CANNOT_CHANGE_MODE_P (REGNO (inner), GET_MODE (inner), mode))
>     return true;
> #endif
>
> to it isn't enough though, since that just forces the inner subreg
> to be reloaded into another FPR and then we simplify _that_ FPR to
> an SImode and reload it.
>
> While this is arguably a bug in reload, the situation isn't really
> supposed to happen in practice, since register_operand specifically
> disallows this kind of subreg:
>
> #ifdef CANNOT_CHANGE_MODE_CLASS
>       if (REG_P (sub)
>           && REGNO (sub) < FIRST_PSEUDO_REGISTER
>           && REG_CANNOT_CHANGE_MODE_P (REGNO (sub), GET_MODE (sub), mode)
>           && GET_MODE_CLASS (GET_MODE (sub)) != MODE_COMPLEX_INT
>           && GET_MODE_CLASS (GET_MODE (sub)) != MODE_COMPLEX_FLOAT
>           /* LRA can generate some invalid SUBREGS just for matched
>              operand reload presentation.  LRA needs to treat them as
>              valid.  */
>           && ! LRA_SUBREG_P (op))
>         return 0;
> #endif
>
> The problem is that nonmemory_operand and general_operand both have
> slightly different ideas about what a register operand should be and
> neither of them has this check.  register_operand also has:
>
>       /* FLOAT_MODE subregs can't be paradoxical.  Combine will occasionally
>          create such rtl, and we must reject it.  */
>       if (SCALAR_FLOAT_MODE_P (GET_MODE (op))
>           /* LRA can use subreg to store a floating point value in an
>              integer mode.  Although the floating point and the
>              integer modes need the same number of hard registers, the
>              size of floating point mode can be less than the integer
>              mode.  */
>           && ! lra_in_progress
>           && GET_MODE_SIZE (GET_MODE (op)) > GET_MODE_SIZE (GET_MODE (sub)))
>         return 0;
>
> which is copied in general_operand but missing from nonmemory_operand.
> This second check doesn't apply to the testcase but it seems unlikely
> that the difference is intentional.
>
> This patch therefore consolidates all the register checks in general_operand
> and makes the others call into it.  Tested on x86_64-linux-gnu and
> mips64-linux-gnu.  OK to install?

Ok.

Thanks,
Richard.

> The original testcase was from the fortran testsuite, so no new one here.
>
> Thanks,
> Richard
>
>
> gcc/
>         PR rtl-optimization/60604
>         * recog.c (general_operand): Incorporate REG_CANNOT_CHANGE_MODE_P
>         check from register_operand.
>         (register_operand): Redefine in terms of general_operand.
>         (nonmemory_operand): Use register_operand for the non-constant cases.
>
> Index: gcc/recog.c
> ===================================================================
> --- gcc/recog.c 2014-03-30 22:18:23.803009353 +0100
> +++ gcc/recog.c 2014-03-30 22:37:47.534721724 +0100
> @@ -1023,6 +1023,19 @@ general_operand (rtx op, enum machine_mo
>           && MEM_P (sub))
>         return 0;
>
> +#ifdef CANNOT_CHANGE_MODE_CLASS
> +      if (REG_P (sub)
> +         && REGNO (sub) < FIRST_PSEUDO_REGISTER
> +         && REG_CANNOT_CHANGE_MODE_P (REGNO (sub), GET_MODE (sub), mode)
> +         && GET_MODE_CLASS (GET_MODE (sub)) != MODE_COMPLEX_INT
> +         && GET_MODE_CLASS (GET_MODE (sub)) != MODE_COMPLEX_FLOAT
> +         /* LRA can generate some invalid SUBREGS just for matched
> +            operand reload presentation.  LRA needs to treat them as
> +            valid.  */
> +         && ! LRA_SUBREG_P (op))
> +       return 0;
> +#endif
> +
>        /* FLOAT_MODE subregs can't be paradoxical.  Combine will occasionally
>          create such rtl, and we must reject it.  */
>        if (SCALAR_FLOAT_MODE_P (GET_MODE (op))
> @@ -1083,9 +1096,6 @@ address_operand (rtx op, enum machine_mo
>  int
>  register_operand (rtx op, enum machine_mode mode)
>  {
> -  if (GET_MODE (op) != mode && mode != VOIDmode)
> -    return 0;
> -
>    if (GET_CODE (op) == SUBREG)
>      {
>        rtx sub = SUBREG_REG (op);
> @@ -1096,41 +1106,12 @@ register_operand (rtx op, enum machine_m
>          (Ideally, (SUBREG (MEM)...) should not exist after reload,
>          but currently it does result from (SUBREG (REG)...) where the
>          reg went on the stack.)  */
> -      if (! reload_completed && MEM_P (sub))
> -       return general_operand (op, mode);
> -
> -#ifdef CANNOT_CHANGE_MODE_CLASS
> -      if (REG_P (sub)
> -         && REGNO (sub) < FIRST_PSEUDO_REGISTER
> -         && REG_CANNOT_CHANGE_MODE_P (REGNO (sub), GET_MODE (sub), mode)
> -         && GET_MODE_CLASS (GET_MODE (sub)) != MODE_COMPLEX_INT
> -         && GET_MODE_CLASS (GET_MODE (sub)) != MODE_COMPLEX_FLOAT
> -         /* LRA can generate some invalid SUBREGS just for matched
> -            operand reload presentation.  LRA needs to treat them as
> -            valid.  */
> -         && ! LRA_SUBREG_P (op))
> -       return 0;
> -#endif
> -
> -      /* FLOAT_MODE subregs can't be paradoxical.  Combine will occasionally
> -        create such rtl, and we must reject it.  */
> -      if (SCALAR_FLOAT_MODE_P (GET_MODE (op))
> -         /* LRA can use subreg to store a floating point value in an
> -            integer mode.  Although the floating point and the
> -            integer modes need the same number of hard registers, the
> -            size of floating point mode can be less than the integer
> -            mode.  */
> -         && ! lra_in_progress
> -         && GET_MODE_SIZE (GET_MODE (op)) > GET_MODE_SIZE (GET_MODE (sub)))
> +      if (!REG_P (sub) && (reload_completed || !MEM_P (sub)))
>         return 0;
> -
> -      op = sub;
>      }
> -
> -  return (REG_P (op)
> -         && (REGNO (op) >= FIRST_PSEUDO_REGISTER
> -             || in_hard_reg_set_p (operand_reg_set,
> -                                   GET_MODE (op), REGNO (op))));
> +  else if (!REG_P (op))
> +    return 0;
> +  return general_operand (op, mode);
>  }
>
>  /* Return 1 for a register in Pmode; ignore the tested mode.  */
> @@ -1232,27 +1213,7 @@ nonmemory_operand (rtx op, enum machine_
>  {
>    if (CONSTANT_P (op))
>      return immediate_operand (op, mode);
> -
> -  if (GET_MODE (op) != mode && mode != VOIDmode)
> -    return 0;
> -
> -  if (GET_CODE (op) == SUBREG)
> -    {
> -      /* Before reload, we can allow (SUBREG (MEM...)) as a register operand
> -        because it is guaranteed to be reloaded into one.
> -        Just make sure the MEM is valid in itself.
> -        (Ideally, (SUBREG (MEM)...) should not exist after reload,
> -        but currently it does result from (SUBREG (REG)...) where the
> -        reg went on the stack.)  */
> -      if (! reload_completed && MEM_P (SUBREG_REG (op)))
> -       return general_operand (op, mode);
> -      op = SUBREG_REG (op);
> -    }
> -
> -  return (REG_P (op)
> -         && (REGNO (op) >= FIRST_PSEUDO_REGISTER
> -             || in_hard_reg_set_p (operand_reg_set,
> -                                   GET_MODE (op), REGNO (op))));
> +  return register_operand (op, mode);
>  }
>
>  /* Return 1 if OP is a valid operand that stands for pushing a