This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Add a partial_subreg_p predicate


Jeff Law <law@redhat.com> writes:
> On 08/21/2017 07:34 AM, Richard Sandiford wrote:
>> This patch adds a partial_subreg_p predicate to go alongside
>> paradoxical_subreg_p.
>> 
>> 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?
>> 
>> Richard
>> 
>> 
>> 2017-08-21  Richard Sandiford  <richard.sandiford@linaro.org>
>> 	    Alan Hayward  <alan.hayward@arm.com>
>> 	    David Sherwood  <david.sherwood@arm.com>
>> 
>> gcc/
>> 	* 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.

Thanks,
Richard

>
> jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]