Jakub Jelinek <ja...@redhat.com> writes:
> On Thu, May 13, 2021 at 05:37:36PM +0200, Jakub Jelinek wrote:
>> So, do you want something like (I've deleted the old comment as I think
>> the new one is enough, but am open to keep both) the patch below, where
>> it REG_CAN_CHANGE_MODE_P is false, we punt (return), otherwise call
>> set_value_regno?
>> Am not sure if those REG_CAN_CHANGE_MODE_P arguments is what you want
>> though.
>
> Oops, missing !, meant following which works on 11 branch for the testcase:
>
> 2021-05-13 Jakub Jelinek <ja...@redhat.com>
>
> PR rtl-optimization/100342
> * regcprop.c (copy_value): When copying a source reg in a wider
> mode than it has recorded for the value, adjust recorded destination
> mode too or punt if !REG_CAN_CHANGE_MODE_P.
>
> * gcc.target/i386/pr100342.c: New test.
>
> --- gcc/regcprop.c.jj 2021-03-23 10:21:07.176447920 +0100
> +++ gcc/regcprop.c 2021-05-13 17:36:46.443192451 +0200
> @@ -358,34 +358,25 @@ copy_value (rtx dest, rtx src, struct va
> else if (sn > hard_regno_nregs (sr, vd->e[sr].mode))
> return;
>
> - /* It is not safe to link DEST into the chain if SRC was defined in some
> - narrower mode M and if M is also narrower than the mode of the first
> - register in the chain. For example:
> - (set (reg:DI r1) (reg:DI r0))
> - (set (reg:HI r2) (reg:HI r1))
> - (set (reg:SI r3) (reg:SI r2)) //Should be a new chain start at r3
> - (set (reg:SI r4) (reg:SI r1))
> - (set (reg:SI r5) (reg:SI r4))
> -
> - the upper part of r3 is undefined. If we added it to the chain,
> - it may be used to replace r5, which has defined upper bits.
> - See PR98694 for details.
> -
> - [A] partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
> - [B] partial_subreg_p (vd->e[sr].mode,
> vd->e[vd->e[sr].oldest_regno].mode)
> - Condition B is added to to catch optimization opportunities of
> -
> - (set (reg:HI R1) (reg:HI R0))
> - (set (reg:SI R2) (reg:SI R1)) // [A]
> - (set (reg:DI R3) (reg:DI R2)) // [A]
> - (set (reg:SI R4) (reg:SI R[0-3]))
> - (set (reg:HI R5) (reg:HI R[0-4]))
> -
> - in which all registers have only 16 defined bits. */
> - else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
> - && partial_subreg_p (vd->e[sr].mode,
> - vd->e[vd->e[sr].oldest_regno].mode))
> - return;
> + /* If a narrower value is copied using wider mode, the upper bits
> + are undefined (could be e.g. a former paradoxical subreg). Signal
> + in that case we've only copied value using the narrower mode.
> + Consider:
> + (set (reg:DI r14) (mem:DI ...))
> + (set (reg:QI si) (reg:QI r14))
> + (set (reg:DI bp) (reg:DI r14))
> + (set (reg:DI r14) (const_int ...))
> + (set (reg:DI dx) (reg:DI si))
> + (set (reg:DI si) (const_int ...))
> + (set (reg:DI dx) (reg:DI bp))
> + The last set is not redundant, while the low 8 bits of dx are already
> + equal to low 8 bits of bp, the other bits are undefined. */
> + else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src)))
> + {
> + if (!REG_CAN_CHANGE_MODE_P (sr, GET_MODE (src), vd->e[sr].mode))
> + return;
LGTM, but for extra safety, I think we also want:
|| !REG_CAN_CHANGE_MODE_P (dr, vd->e[sr].mode, GET_MODE (dst))
i.e. we're effectively converting the source from the wider mode
to the narrower mode, doing a move, and then converting the narrow
mode back to the wider mode.
OK with that change, thanks.
Richard
