Jeff Law <l...@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.sandif...@linaro.org> >> Alan Hayward <alan.hayw...@arm.com> >> David Sherwood <david.sherw...@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
