Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].

[Richard Sandiford <richard.sandiford@arm.com>]

Hi Vlad,

Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On 2022-02-14 04:44, Richard Sandiford via Gcc-patches wrote:
>> Iain Sandoe via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>> Two issues resulted in this PR, which manifests when we force a constant into
>>> memory in LRA (in PIC code on Darwin).  The presence of such forced constants
>>> is quite dependent on other RTL optimisations, and it is easy for the issue to
>>> become latent for a specific case.
>>>
>>> First, in the Darwin-specific rs6000 backend code, we were not being careful
>>> enough in rejecting invalid symbolic addresses.  Specifically, when generating
>>> PIC code, we require a SYMBOL_REF to be wrapped in an UNSPEC_MACHOPIC_OFFSET.
>>>
>>> Second, LRA was attempting to load a register using an invalid lo_sum address.
>>>
>>> The LRA changes are approved in the PR by Vladimir, and the RS6000 changes are
>>> Darwin-specific (although, of course, any observations are welcome).
>>>
>>> Tested on several lo_sum targets and x86_64 all languages except as noted:
>>> powerpc64-linux (m32/m64) -D
>>> powerpc64le-linux  -D
>>> powerpc64-aix -Ada -Go -D
>>> aarch64-linux -Ada -D
>>> x86_64-linux all langs -D
>>> powerpc-darwin9 (master and 11.2) -D -Go.
>>>
>>> pushed to master, thanks,
>>> Iain
>>>
>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>> Co-authored-by: Vladimir Makarov <vmakarov@redhat.com>
>>>
>>> 	PR target/104117
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* config/rs6000/rs6000.cc (darwin_rs6000_legitimate_lo_sum_const_p):
>>> 	Check for UNSPEC_MACHOPIC_OFFSET wrappers on symbolic addresses when
>>> 	emitting PIC code.
>>> 	(legitimate_lo_sum_address_p): Likewise.
>>> 	* lra-constraints.cc (process_address_1): Do not attempt to emit a reg
>>> 	load from an invalid lo_sum address.
>>> ---
>>>   gcc/config/rs6000/rs6000.cc | 38 +++++++++++++++++++++++++++++++++++--
>>>   gcc/lra-constraints.cc      | 17 ++---------------
>>>   2 files changed, 38 insertions(+), 17 deletions(-)
>>>
>>> […]
>>> diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
>>> index fdff9e0720a..c700c3f4578 100644
>>> --- a/gcc/lra-constraints.cc
>>> +++ b/gcc/lra-constraints.cc
>>> @@ -3625,21 +3625,8 @@ process_address_1 (int nop, bool check_only_p,
>>>   		  *ad.inner = gen_rtx_LO_SUM (Pmode, new_reg, addr);
>>>   		  if (!valid_address_p (op, &ad, cn))
>>>   		    {
>>> -		      /* Try to put lo_sum into register.  */
>>> -		      insn = emit_insn (gen_rtx_SET
>>> -					(new_reg,
>>> -					 gen_rtx_LO_SUM (Pmode, new_reg, addr)));
>>> -		      code = recog_memoized (insn);
>>> -		      if (code >= 0)
>>> -			{
>>> -			  *ad.inner = new_reg;
>>> -			  if (!valid_address_p (op, &ad, cn))
>>> -			    {
>>> -			      *ad.inner = addr;
>>> -			      code = -1;
>>> -			    }
>>> -			}
>>> -
>>> +		      *ad.inner = addr; /* Punt.  */
>>> +		      code = -1;
>>>   		    }
>>>   		}
>>>   	      if (code < 0)
>> Could you go into more details about this?  Why is it OK to continue
>> to try:
>>
>>    (lo_sum new_reg addr)
>>
>> directly as an address (the context at the top of the hunk), but not try
>> moving the lo_sum into a register?  They should be semantically equivalent,
>> so it seems that if one is wrong, the other would be too.
>>
> Hi, Richard.  Change LRA is mine and I approved it for Iain's patch.
>
> I think there is no need for this code and it is misleading.  If 
> 'mem[low_sum]' does not work, I don't think that 'reg=low_sum;mem[reg]' 
> will help for any existing target.  As machine-dependent code for any 
> target most probably (for ppc64 darwin it is exactly the case) checks 
> address only in memory, it can wrongly accept wrong address by reloading 
> it into reg and use it in memory. So these are my arguments for the 
> remove this code from process_address_1.

I'm probably making too much of this, but:

I think the code is potentially useful in that existing targets do forbid
forbid lo_sum addresses in certain contexts (due to limited offset range)
while still wanting lo_sum to be used to be load the address.  If we
handle the high/lo_sum split in generic code then we have more chance
of being able to optimise things.  So it feels like this is setting an
unfortunate precedent.

I still don't understand what went wrong before though (the PR trail
was a bit too long to process :-)).  Is there a case where
(lo_sum (high X) X) != X?  If so, that seems like a target bug to me.
Or does the target accept (set R1 (lo_sum R2 X)) for an X that cannot
be split into a HIGH/LO_SUM pair?  I'd argue that's a target bug too.

Thanks,
Richard