This is the mail archive of the
mailing list for the GCC project.
Re: Add a partial_subreg_p predicate
- From: Richard Sandiford <richard dot sandiford at linaro dot org>
- To: Jeff Law <law at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Wed, 30 Aug 2017 16:41:24 +0100
- Subject: Re: Add a partial_subreg_p predicate
- Authentication-results: sourceware.org; auth=none
- References: <email@example.com> <firstname.lastname@example.org>
Jeff Law <email@example.com> writes:
> On 08/21/2017 07:34 AM, Richard Sandiford wrote:
>> This patch adds a partial_subreg_p predicate to go alongside
>> The first two changes to cse_insn preserve the current behaviour,
>> but the condition seems strange. Shouldn't we be able to continue
>> to cse if the inner modes of the two subregs have the same size?
> I would think so. It could well be a simple oversight. We're talking
> about very old code -- this stuff is virtually unchanged in 25 years.
> This specific chunk of code is something I would like to look more
> deeply into its utility -- most of what it's doing should be handled by
> DOM these days. It'd be an interesting exercise to see if this code is
> important at all anymore.
OK. When I get some spare machine time, I might try putting a
gcc_unreachable () there and doing the usual multi-target testing,
just to see if or when it fires.
> If you wanted to handle that case, I wouldn't object as a follow-up if
> you can come up with a testcase that shows when its useful.
>> The patch also preserves the existing condition in
>> simplify_operand_subreg, but perhaps it should be using
>> a df_read_modify_subreg_p-style check instead. We don't
>> need to reload the inner value first if the inner value
>> is no bigger than a word, for example.
> Not sure on this one. We could try to change it as a follow-up though.
> Vlad may have a better sense of how this might interact with other parts
> of LRA than I would
I'll give this one a go too.
>> The use in curr_insn_transform also seemed strange. Surely the
>> modes of the SET_DEST and SET_SRC will be the same, given that
>> this code isn't meant for constants?
> Is it possible this is for the zero/sign extension support?
Even then I don't think we could ever have SET_SRC and SET_DEST being
different. The extension has to be explicit in the source.
>> Like the paradoxical_subreg_p patch, this one replaces some tests that
>> were based on GET_MODE_SIZE rather than GET_MODE_PRECISION. In each
>> case the change should be a no-op or an improvement.
>> Doing this in regcprop.c prevents some replacements of the 82-bit RFmode
>> with the 80-bit XFmode on ia64. I don't understand the target details
>> here particularly well, but from the way the values are described in
>> ia64-modes.def, it isn't valid to assume that an XFmode can carry an
>> RFmode payload. A comparison of the testsuite assembly output for one
>> target per CPU showed no other differences.
>> Some of the places changed here are tracking the widest access mode
>> found for a register. The series tries to standardise on:
>> if (partial_subreg_p (widest_seen, new_mode))
>> widest_seen = new_mode;
>> rather than:
>> if (paradoxical_subreg_p (new_mode, widest_seen))
>> widest_seen = new_mode;
>> Either would have been OK.
>> Tested on aarch64-linux-gnu and x86_64-linux-gnu in addition to the above.
>> OK to install?
>> 2017-08-21 Richard Sandiford <firstname.lastname@example.org>
>> Alan Hayward <email@example.com>
>> David Sherwood <firstname.lastname@example.org>
>> * rtl.h (partial_subreg_p): New function.
>> * caller-save.c (save_call_clobbered_regs): Use it.
>> * calls.c (expand_call): Likewise.
>> * combine.c (combinable_i3pat): Likewise.
>> (simplify_set): Likewise.
>> (make_extraction): Likewise.
>> (make_compound_operation_int): Likewise.
>> (gen_lowpart_or_truncate): Likewise.
>> (force_to_mode): Likewise.
>> (make_field_assignment): Likewise.
>> (reg_truncated_to_mode): Likewise.
>> (record_truncated_value): Likewise.
>> (move_deaths): Likewise.
>> * cse.c (record_jump_cond): Likewise.
>> (cse_insn): Likewise.
>> * cselib.c (cselib_lookup_1): Likewise.
>> * df-scan.c (df_read_modify_subreg_p): Likewise.
>> * expmed.c (extract_bit_field_using_extv): Likewise.
>> * function.c (assign_parm_setup_reg): Likewise.
>> * ifcvt.c (noce_convert_multiple_sets): Likewise.
>> * ira-build.c (create_insn_allocnos): Likewise.
>> * lra-coalesce.c (merge_pseudos): Likewise.
>> * lra-constraints.c (match_reload): Likewise.
>> (simplify_operand_subreg): Likewise.
>> (curr_insn_transform): Likewise.
>> * lra-lives.c (process_bb_lives): Likewise.
>> * lra.c (new_insn_reg): Likewise.
>> (lra_substitute_pseudo): Likewise.
>> * regcprop.c (mode_change_ok): Likewise.
>> (maybe_mode_change): Likewise.
>> (copyprop_hardreg_forward_1): Likewise.
>> * reload.c (push_reload): Likewise.
>> (find_reloads): Likewise.
>> (find_reloads_subreg_address): Likewise.
>> * reload1.c (alter_reg): Likewise.
>> (eliminate_regs_1): Likewise.
>> * simplify-rtx.c (simplify_unary_operation_1): Likewise.
> Any thought on whether or not a same size subreg predicate would be
> useful. ie, (subreg:SI (reg:SF)) and the like. Certainly not something
> you have to do to go forward, just something to ponder.
Yeah, it might be worth having that too if there are a few instances
of it. I'll keep an eye out.
> The patch is OK for the trunk. The issues noted above all seem like
> potentially follow-up items if we want to tackle them at all.