On 2022-02-14 11:00, Richard Sandiford wrote:
Hi Vlad,
Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
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.
Sometimes it is hard to make a line where an RA bug is a bug in
machine-dependent code or in RA itself.
For this case I would say it is a bug in the both parts.
Low-sum is generated by LRA and it does not know that it should be
wrapped by unspec for darwin. Generally speaking we could avoid the
change in LRA but it would require to do non-trivial analysis in machine
dependent code to find cases when 'reg=low_sum ... mem[reg]' is
incorrect code for darwin (PIC) target (and may be some other PIC
targets too). Therefore I believe the change in LRA is a good solution
even if the change can potentially result in less optimized code for
some cases. Taking your concern into account we could probably improve
the patch by introducing a hook (I never liked such solutions as we
already have too many hooks directing RA) or better to make the LRA
change working only for PIC target. Something like this (it probably
needs better recognition of pic target):
--- a/gcc/lra-constraints.cc
+++ b/gcc/lra-constraints.cc
@@ -3616,21 +3616,21 @@ process_address_1 (int nop, bool check_only_p,
if (HAVE_lo_sum)
{
/* addr => lo_sum (new_base, addr), case (2) above. */
insn = emit_insn (gen_rtx_SET
(new_reg,
gen_rtx_HIGH (Pmode, copy_rtx (addr))));
code = recog_memoized (insn);
if (code >= 0)
{
*ad.inner = gen_rtx_LO_SUM (Pmode, new_reg, addr);
- if (!valid_address_p (op, &ad, cn))
+ if (!valid_address_p (op, &ad, cn) && !flag_pic)
{
/* 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))
