Re: [PATCH] cprop_hardreg: Workaround for narrow mode != lowpart targets

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

Andreas Krebbel via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On 1/14/22 20:41, Andreas Krebbel via Gcc-patches wrote:
>> On 1/14/22 08:37, Richard Biener wrote:
>> ...
>>> Can the gist of this bug be put into the GCC bugzilla so the rev can
>>> refer to it? 
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104034
>> 
>>> Can we have a testcase even?
>> The testcase from Jakub is in the BZ. However, since it doesn't fail with head I didn't try to
>> include it in my patch.
>> 
>>> I'm not quite understanding the problem but is it that, say,
>>>
>>>  (subreg:DI (reg:V2DI ..) 0)
>>>
>>> isn't the same as
>>>
>>>  (lowpart:DI (reg:V2DI ...) 0)
>> 
>> (reg:DI v0) does not match the lower order bits of (reg:TI v0)
>> 
>>> ?  The regcprop code looks more like asking whether the larger reg
>>> is a composition of multiple other hardregs and will return the specific
>>> hardreg corresponding to the lowpart - so like if on s390 the vector
>>> registers overlap with some other regset.  But then doing the actual
>>> accesses via the other regset regs doesn't actually work?  Isn't the
>>> backend then lying to us (aka the mode_change_ok returns the
>>> wrong answer)?
>> 
>> can_change_mode_class should do the right thing. We return false in case somebody wants to change TI
>> to DI for a vector register. However, the hook never gets called like this from regcprop. regcprop
>> only asks whether it is ok to change (reg:TI r8) to (reg:DI r8) and that's indeed ok.
>
> After writing this I'm wondering whether this would be a better fix:
>
> diff --git a/gcc/regcprop.c b/gcc/regcprop.c
> index 18132425ab2..b6a3f4e3804 100644
> --- a/gcc/regcprop.c
> +++ b/gcc/regcprop.c
> @@ -402,7 +402,8 @@ maybe_mode_change (machine_mode orig_mode, machine_mode copy_mode,
>
>    if (orig_mode == new_mode)
>      return gen_raw_REG (new_mode, regno);
> -  else if (mode_change_ok (orig_mode, new_mode, regno))
> +  else if (mode_change_ok (orig_mode, new_mode, regno)
> +           && mode_change_ok (copy_mode, new_mode, copy_regno))
>      {
>        int copy_nregs = hard_regno_nregs (copy_regno, copy_mode);
>        int use_nregs = hard_regno_nregs (copy_regno, new_mode);
>

Yeah, this looks good to me FWIW.

Richard