| Message ID | 20170222202522.GK14945@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
Alan Modra <amodra@gmail.com> writes: > This patch allows lra to reload a lo_sum address. Given a mem with a > lo_sum address as used by ppc32, decompose_mem_address returns an > address_info with *base the lo_sum and *base_term the reg within the > lo_sum. When a lo_sum address isn't valid, we want to first try > reloading the entire lo_sum to a new reg. I first fixed that, then > hit another ICE, this time in gen_int_mode. gen_int_mode was being > called with the mode of the memory access, not the mode of the > address. It seems odd that we have a lo_sum address for a mode that doesn't accept lo_sums. I don't think that was one of the "three" cases anticapted in: /* There are three cases where the shape of *AD.INNER may now be invalid: 1) the original address was valid, but either elimination or equiv_address_substitution was applied and that made the address invalid. 2) the address is an invalid symbolic address created by force_const_to_mem. 3) the address is a frame address with an invalid offset. 4) the address is a frame address with an invalid base. All these cases involve a non-autoinc address, so there is no point revalidating other types. */ It sounds from the PR comments like a valid lo_sum in a mem:SI somehow becomes an invalid lo_sum in a mem:SD through equivalence, is that right? If so, was there a (subreg:SD (reg:SI ...)) in the pre-RA insn that got "simplifed" to a (mem:SD (lo_sum ...)) during LRA? IMO the bug's in that step if so and if lo_sum isn't valid for mem:SD. The patch instead seems to be moving towards reloading any address we end up with (which presumably fits one of the options above for _some_ mode, just not in this case for the current mode). But then there's no reason why the remaining assert: > @@ -2916,18 +2916,18 @@ base_to_reg (struct address_info *ad) > rtx_insn *insn; > rtx_insn *last_insn = get_last_insn(); > > - lra_assert (ad->base == ad->base_term && ad->disp == ad->disp_term); > + lra_assert (ad->disp == ad->disp_term); would hold either. We'd need to make base_to_reg handle every address recognised by decompose_mem_address, including for instance cases in which an index could be sign-extended for the original mem mode but not for the current one. I don't think the code was ever intended to handle anything that complicated. The address has to be "basically" valid, except in the cases above. Thanks, Richard
On Wed, Feb 22, 2017 at 11:01:15PM +0000, Richard Sandiford wrote: > Alan Modra <amodra@gmail.com> writes: > > This patch allows lra to reload a lo_sum address. Given a mem with a > > lo_sum address as used by ppc32, decompose_mem_address returns an > > address_info with *base the lo_sum and *base_term the reg within the > > lo_sum. When a lo_sum address isn't valid, we want to first try > > reloading the entire lo_sum to a new reg. I first fixed that, then > > hit another ICE, this time in gen_int_mode. gen_int_mode was being > > called with the mode of the memory access, not the mode of the > > address. > > It seems odd that we have a lo_sum address for a mode that doesn't > accept lo_sums. I don't think that was one of the "three" cases > anticapted in: > > /* There are three cases where the shape of *AD.INNER may now be invalid: > > 1) the original address was valid, but either elimination or > equiv_address_substitution was applied and that made > the address invalid. > > 2) the address is an invalid symbolic address created by > force_const_to_mem. > > 3) the address is a frame address with an invalid offset. > > 4) the address is a frame address with an invalid base. > > All these cases involve a non-autoinc address, so there is no > point revalidating other types. */ > > It sounds from the PR comments like a valid lo_sum in a mem:SI > somehow becomes an invalid lo_sum in a mem:SD through equivalence, > is that right? Yes, that it correct. > If so, was there a (subreg:SD (reg:SI ...)) in the > pre-RA insn that got "simplifed" to a (mem:SD (lo_sum ...)) during LRA? Yes. The insn in question out of ira is this one: (insn 11 7 10 2 (set (reg:SD 33 1) (subreg:SD (reg:SI 155 [ i.0_1 ]) 0)) "/home/alan/src/gcc.git/gcc/testsuite/c-c++-common/dfp/pr35620.c":20 489 {movsd_hardfloat} (nil)) (reg:SI 155) has the equivalence with (mem/j/c:SI (lo_sum:SI (reg/f:SI 160) (symbol_ref:SI ("u")))) At the time we fail valid_address_p, lra is looking at (insn 11 17 10 2 (set (reg:SD 33 1) (mem/j/c:SD (lo_sum:SI (reg/f:SI 160) (symbol_ref:SI ("u") [flags 0x84] <var_decl 0x7ffff7ff5510 u>)) [2 u.b+0 S4 A32])) "/home/alan/src/gcc.git/gcc/testsuite/c-c++-common/dfp/pr35620.c":20 489 {movsd_hardfloat} > IMO the bug's in that step if so and if lo_sum isn't valid for mem:SD. lo_sum is indeed not valid for mem:SD. simplify_operand_subreg is where the subreg disappears. [snip] > I don't think the code was ever intended to handle anything > that complicated. The address has to be "basically" valid, > except in the cases above. I'm seriously regretting my foray into ira.c, attempting to modernize just a small part of the code to use the DF infrastructure..
On Thu, Feb 23, 2017 at 11:41:09AM +1030, Alan Modra wrote: > lo_sum is indeed not valid for mem:SD. simplify_operand_subreg is > where the subreg disappears. Richard, doesn't the following say that lra is expecting to reload exactly the lo_sum address you seem to think it should not handle in process_address? /* We still can reload address and if the address is valid, we can remove subreg without reloading its inner memory. */ && valid_address_p (GET_MODE (subst), regno_reg_rtx [ira_class_hard_regs [base_reg_class (GET_MODE (subst), MEM_ADDR_SPACE (subst), ADDRESS, SCRATCH)][0]], MEM_ADDR_SPACE (subst))))
Alan Modra <amodra@gmail.com> writes: > On Thu, Feb 23, 2017 at 11:41:09AM +1030, Alan Modra wrote: >> lo_sum is indeed not valid for mem:SD. simplify_operand_subreg is >> where the subreg disappears. > > Richard, doesn't the following say that lra is expecting to reload > exactly the lo_sum address you seem to think it should not handle in > process_address? > > /* We still can reload address and if the address is > valid, we can remove subreg without reloading its > inner memory. */ > && valid_address_p (GET_MODE (subst), > regno_reg_rtx > [ira_class_hard_regs > [base_reg_class (GET_MODE (subst), > MEM_ADDR_SPACE (subst), > ADDRESS, SCRATCH)][0]], > MEM_ADDR_SPACE (subst)))) Yeah, I think that's a bit too broad. It was added in: 2016-02-03 Vladimir Makarov <vmakarov@redhat.com> Alexandre Oliva <aoliva@redhat.com> PR target/69461 * lra-constraints.c (simplify_operand_subreg): Check additionally address validity after potential reloading. (process_address_1): Check insns validity. In case of failure do nothing. to allow the subreg to be simplified for the stack mem: (mem/c:V2DF (plus:DI (reg/f:DI 113 sfp) (const_int 96 [0x60])) [6 %sfp+96 S16 A128]) Coping with that kind of stack address is no problem. But the patch seems to allow any other address through as well, even though process_address_1 still has the old assumption that the address is "basically" valid. E.g. as well as the assert you were patching, there's: /* Any index existed before LRA started, so we can assume that the presence and shape of the index is valid. */ push_to_sequence (*before); lra_assert (ad.disp == ad.disp_term); which could also fire if we allow an address that has the right shape for one mode to be used with any other mode. The patch made process_address_1 punt and return false for the MEM quoted above. I think it'd be dangerous to extend that to all other types of MEM. Wouldn't that require the target to provide a load-address pattern for every possible MEM address? E.g. if the address requires relocation operators, the load-address version might need a different relocation sequence from the load/store version. Thanks, Richard
I'm going to wait for Vlad's opinion. I've written a couple of replies and erased them, since I figure whatever I have to say doesn't carry much weight.
On 02/24/2017 12:07 AM, Alan Modra wrote: > I'm going to wait for Vlad's opinion. I've written a couple of > replies and erased them, since I figure whatever I have to say doesn't > carry much weight. > I would prefer not to touch simplify_subreg_operand, especially a code related to subreg of mem. Changes of the code usually result in a long time stabilization work. The change might result in LRA cycling (I remember it happened before exactly for ppc SD memory handling). It is dangerous when we are closer to the release. I hope more work on clearing the simplify_subreg_operand code will be done for GCC-8 (at least Matt Fortune expressed his willingness to do this). Although it is better to avoid a wrong RTL code creation earlier, LRA already can create a wrong code during its work for a long period of time. It already happens for some cases. So please commit your patch, Allan. But before doing it also please check that the patch does not creates problems on x86-64 bootstrap and GCC tests. Thank you.
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index bd5fbcd..0e98f22 100644 --- a/gcc/lra-constraints.c +++ b/gcc/lra-constraints.c @@ -2916,18 +2916,18 @@ base_to_reg (struct address_info *ad) rtx_insn *insn; rtx_insn *last_insn = get_last_insn(); - lra_assert (ad->base == ad->base_term && ad->disp == ad->disp_term); + lra_assert (ad->disp == ad->disp_term); cl = base_reg_class (ad->mode, ad->as, ad->base_outer_code, get_index_code (ad)); - new_reg = lra_create_new_reg (GET_MODE (*ad->base_term), NULL_RTX, + new_reg = lra_create_new_reg (GET_MODE (*ad->base), NULL_RTX, cl, "base"); new_inner = simplify_gen_binary (PLUS, GET_MODE (new_reg), new_reg, ad->disp_term == NULL - ? gen_int_mode (0, ad->mode) + ? const0_rtx : *ad->disp_term); if (!valid_address_p (ad->mode, new_inner, ad->as)) return NULL_RTX; - insn = emit_insn (gen_rtx_SET (new_reg, *ad->base_term)); + insn = emit_insn (gen_rtx_SET (new_reg, *ad->base)); code = recog_memoized (insn); if (code < 0) {