On 10/13/2017 10:08 AM, Richard Sandiford wrote: > 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. OK. Thanks for clarifying.
Jeff
