Re: [PATCH] regcprop: Fix another cprop_hardreg bug [PR100342]

[Richard Sandiford <richard.sandiford@arm.com>]

Jakub Jelinek <jakub@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  <jakub@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