[PATCH] PR79584, lra ICE in base_to_reg

[PATCH] PR79584, lra ICE in base_to_reg

[Alan Modra]

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.

Bootstrapped and regression tested powerpc64le-linux and
powerpc64-linux bi-arch.  OK to apply?

	PR rtl-optimization/79584
	* lra-constraints.c (base_to_reg): Reload ad->base, the entire
	base, not ad->base_term, the reg within base.  Remove assertion
	that ad->base == ad->base_term.  Replace gen_int_mode using
	bogus mode with const0_rtx.

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)
     {

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR79584, lra ICE in base_to_reg

[Richard Sandiford]

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

Re: [PATCH] PR79584, lra ICE in base_to_reg

[Alan Modra]

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..

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR79584, lra ICE in base_to_reg

[Alan Modra]

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
Australia Development Lab, IBM

Re: [PATCH] PR79584, lra ICE in base_to_reg

[Richard Sandiford]

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

Re: [PATCH] PR79584, lra ICE in base_to_reg

[Alan Modra]

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.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR79584, lra ICE in base_to_reg

[Vladimir Makarov]

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.