On Mon, Mar 31, 2014 at 6:42 PM, Richard Sandiford
<rdsandif...@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
