This is the mail archive of the
mailing list for the GCC project.
Re: Add a partial_subreg_p predicate
- From: Jeff Law <law at redhat dot com>
- To: gcc-patches at gcc dot gnu dot org, richard dot sandiford at linaro dot org
- Date: Tue, 29 Aug 2017 15:49:23 -0600
- Subject: Re: Add a partial_subreg_p predicate
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=law at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2C9DD4A707
- References: <email@example.com>
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.
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
> 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?
> 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.
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.