Re: [PATCH] PR79584, lra ICE in base_to_reg

[Richard Sandiford <rdsandiford@googlemail.com>]

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