Jeff Law <l...@redhat.com> writes:
> On 08/24/2017 12:25 PM, Richard Sandiford wrote:
>> Segher Boessenkool <seg...@kernel.crashing.org> writes:
>>> On Wed, Aug 23, 2017 at 11:49:03AM +0100, Richard Sandiford wrote:
>>>> This patch uses df_read_modify_subreg_p to check whether writing
>>>> to a subreg would preserve some of the existing contents.
>>>
>>> combine does not keep the DF info up-to-date -- but that is no
>>> problem here, df_read_modify_subreg_p uses no DF info at all. Maybe
>>> it should not have "df_" in the name?
>>
>> Yeah, I guess that's a bit confusing. I've just posted a patch
>> to rename it.
>>
>> Here's a version of the patch that applies on top of that one.
>> Tested as before. OK to install?
>>
>> Thanks,
>> Richard
>>
>>
>> 2017-08-24 Richard Sandiford <richard.sandif...@linaro.org>
>> Alan Hayward <alan.hayw...@arm.com>
>> David Sherwood <david.sherw...@arm.com>
>>
>> gcc/
>> * caller-save.c (mark_referenced_regs): Use read_modify_subreg_p.
>> * combine.c (find_single_use_1): Likewise.
>> (expand_field_assignment): Likewise.
>> (move_deaths): Likewise.
>> * lra-constraints.c (simplify_operand_subreg): Likewise.
>> (curr_insn_transform): Likewise.
>> * lra.c (collect_non_operand_hard_regs): Likewise.
>> (add_regs_to_insn_regno_info): Likewise.
>> * rtlanal.c (reg_referenced_p): Likewise.
>> (covers_regno_no_parallel_p): Likewise.
>>
>
>
>> Index: gcc/combine.c
>> ===================================================================
>> --- gcc/combine.c 2017-08-24 19:22:26.163269637 +0100
>> +++ gcc/combine.c 2017-08-24 19:22:45.218100970 +0100
>> @@ -579,10 +579,7 @@ find_single_use_1 (rtx dest, rtx *loc)
>> && !REG_P (SET_DEST (x))
>> && ! (GET_CODE (SET_DEST (x)) == SUBREG
>> && REG_P (SUBREG_REG (SET_DEST (x)))
>> - && (((GET_MODE_SIZE (GET_MODE (SUBREG_REG (SET_DEST (x))))
>> - + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD)
>> - == ((GET_MODE_SIZE (GET_MODE (SET_DEST (x)))
>> - + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD))))
>> + && !read_modify_subreg_p (SET_DEST (x))))
>> break;
> Is this correct for a paradoxical subreg? ISTM the original code was
> checking for a subreg that just changes the mode, but not the size
> (subreg:SI (reg:SF)) or (subreg:DF (reg:DI)) kinds of things. It would
> reject a paradoxical AFAICT.
>
> As written now I think the condition would be true for a paradoxical.
>
> Similarly for the other two instances in combine.c and the changes in
> rtlanal.c.
>
> In some of those cases you might be able to argue that it's the right
> way to handle a paradoxical. I haven't thought a whole lot about that
> angle, but mention it as a possible way your change might still be correct.
Yeah, I agree this'll change the handling of paradoxical subregs that
occupy more words than the SUBREG_REG, but I think the new version is
correct. The comment says:
/* If the destination is anything other than CC0, PC, a REG or a SUBREG
of a REG that occupies all of the REG, the insn uses DEST if
it is mentioned in the destination or the source. Otherwise, we
need just check the source. */
and a paradoxical subreg does occupy all of the SUBREG_REG.
The code is trying to work out whether the instruction "reads" the
destination if you view partial stores as a read of the old value
followed by a write of a partially-updated value, whereas writing to a
paradoxical subreg preserves none of the original value. And that's
also the semantics that the current code uses for "normal" word-sized
paradoxical subregs.
Thanks,
Richard
