Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
Hi Iain, On Thu, Feb 24, 2022 at 04:02:30PM +, Iain Sandoe wrote: > > On 22 Feb 2022, at 14:44, Vladimir Makarov wrote: > > On 2022-02-20 12:34, Iain Sandoe wrote: > >> > >> ^^^ this is mostly for my education - the stuff below is a potential > >> solution to leaving lra-constraints unchanged and fixing the Darwin bug…. > >> > > I'd be really glad if you do manage to fix this w/o changing LRA. Richard > > has a legitimate point that my proposed change in LRA prohibiting > > `...;reg=low_sum; ...mem[reg]` might force LRA to generate less optimized > > code or even might make LRA to generate unrecognized insns `reg = orginal > > addr` for some ports requiring further fixes in machine-dependent code of > > the ports. > > I think this is within my remit to push without further review - however I’d > very much welcome any comment you folks have: I’d like to push this before my > weekly Darwin test run - which is usually started just after the daily bump > on Saturday morning. > > The other RS6000 changes remain, as Vlad pointed out we were not being picky > enough there - despite getting away with it for longer than I’ve been on the > project ;) > > I tested that the patch fixes the problem on 11.2 (for the testcases > provided, the bug is latent on master) and causes no regressions on > powerpc-darwin9 (master). Nothing in the patch does anything if TARGET_MACHO isn't true, so it is all fine with me. It does look good to me fwiw (the empty constraints are a bit nasty, but they aren't new). Okay for trunk wrt rs6000. Thanks! Segher
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
Folks,
> On 22 Feb 2022, at 14:44, Vladimir Makarov wrote:
>
>
> On 2022-02-20 12:34, Iain Sandoe wrote:
>>
>> ^^^ this is mostly for my education - the stuff below is a potential
>> solution to leaving lra-constraints unchanged and fixing the Darwin bug….
>>
> I'd be really glad if you do manage to fix this w/o changing LRA. Richard has
> a legitimate point that my proposed change in LRA prohibiting
> `...;reg=low_sum; ...mem[reg]` might force LRA to generate less optimized
> code or even might make LRA to generate unrecognized insns `reg = orginal
> addr` for some ports requiring further fixes in machine-dependent code of the
> ports.
I think this is within my remit to push without further review - however I’d
very much welcome any comment you folks have: I’d like to push this before my
weekly Darwin test run - which is usually started just after the daily bump on
Saturday morning.
The other RS6000 changes remain, as Vlad pointed out we were not being picky
enough there - despite getting away with it for longer than I’ve been on the
project ;)
I tested that the patch fixes the problem on 11.2 (for the testcases provided,
the bug is latent on master) and causes no regressions on powerpc-darwin9
(master).
cheers
Iain
[PATCH] LRA, rs6000, Darwin: Revise lo_sum use for forced constants [PR104117].
Follow up discussion to the initial patch for this PR identified that it is
preferable to avoid the LRA change, and arrange for the target to reject the
hi and lo_sum selections when presented with an invalid address.
We split the Darwin high/low selectors into two:
1. One that handles non-PIC addresses (kernel mode, mdynamic-no-pic).
2. One that handles PIC addresses and rejects SYMBOL_REFs unless they are
suitably wrapped in the MACHOPIC_OFFSET unspec.
The second case is handled by providing a new predicate (macho_pic_address)
that checks the requirements.
Signed-off-by: Iain Sandoe
PR target/PR104117
gcc/ChangeLog:
* config/rs6000/darwin.md (@machopic_high_): New.
(@machopic_low_): New.
* config/rs6000/predicates.md (macho_pic_address): New.
* config/rs6000/rs6000.cc (rs6000_legitimize_address): Do not
apply the TLS processing to Darwin.
* lra-constraints.cc (process_address_1): Revert the changes
in r12-7209.
---
gcc/config/rs6000/darwin.md | 19 +++
gcc/config/rs6000/predicates.md | 14 ++
gcc/config/rs6000/rs6000.cc | 2 +-
gcc/lra-constraints.cc | 17 +++--
4 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/gcc/config/rs6000/darwin.md b/gcc/config/rs6000/darwin.md
index 8443585df00..e73d59e8066 100644
--- a/gcc/config/rs6000/darwin.md
+++ b/gcc/config/rs6000/darwin.md
@@ -121,21 +121,32 @@ You should have received a copy of the GNU General Public
License
stw %0,lo16(%2)(%1)"
[(set_attr "type" "store")])
-;; 64-bit MachO load/store support
-
;; Mach-O PIC.
(define_insn "@macho_high_"
[(set (match_operand:P 0 "gpc_reg_operand" "=b*r")
(high:P (match_operand 1 "" "")))]
- "TARGET_MACHO && (DEFAULT_ABI == ABI_DARWIN)"
+ "TARGET_MACHO && (DEFAULT_ABI == ABI_DARWIN) && !flag_pic"
"lis %0,ha16(%1)")
(define_insn "@macho_low_"
[(set (match_operand:P 0 "gpc_reg_operand" "=r")
(lo_sum:P (match_operand:P 1 "gpc_reg_operand" "b")
(match_operand 2 "" "")))]
- "TARGET_MACHO && (DEFAULT_ABI == ABI_DARWIN)"
+ "TARGET_MACHO && (DEFAULT_ABI == ABI_DARWIN) && !flag_pic"
+ "la %0,lo16(%2)(%1)")
+
+(define_insn "@machopic_high_"
+ [(set (match_operand:P 0 "gpc_reg_operand" "=b*r")
+ (high:P (match_operand 1 "macho_pic_address" "")))]
+ "TARGET_MACHO && flag_pic"
+ "lis %0,ha16(%1)")
+
+(define_insn "@machopic_low_"
+ [(set (match_operand:P 0 "gpc_reg_operand" "=r")
+ (lo_sum:P (match_operand:P 1 "gpc_reg_operand" "b")
+ (match_operand 2 "macho_pic_address" "")))]
+ "TARGET_MACHO && flag_pic"
"la %0,lo16(%2)(%1)")
(define_split
diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
index c65dfb91f3d..28f6e9883cb 100644
--- a/gcc/config/rs6000/predicates.md
+++ b/gcc/config/rs6000/predicates.md
@@ -2045,3 +2045,17 @@
(if_then_else (match_test "TARGET_VSX")
(match_operand 0 "reg_or_cint_operand")
(match_operand 0 "const_int_operand")))
+
+;; Return true if the operand is a valid Mach-O pic address.
+;;
+(define_predicate "macho_pic_address"
+ (match_code "const,unspec")
+{
+ if (GET_CODE (op) == CONST)
+op = XEXP (op, 0);
+
+ if (GET_CODE (op) == UNSPEC && XINT (op, 1) == UNSPEC_MACHOPIC_OFFSET)
+return CONSTANT_P (XVECEXP (op, 0, 0));
+ else
+return false;
+})
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index a855e8c4c72..9dbab1fc644 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -9028,7 +9028,7 @@ rs6000_legitimize_address (rtx x, rtx oldx
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
On 2022-02-20 12:34, Iain Sandoe wrote: ^^^ this is mostly for my education - the stuff below is a potential solution to leaving lra-constraints unchanged and fixing the Darwin bug…. I'd be really glad if you do manage to fix this w/o changing LRA. Richard has a legitimate point that my proposed change in LRA prohibiting `...;reg=low_sum; ...mem[reg]` might force LRA to generate less optimized code or even might make LRA to generate unrecognized insns `reg = orginal addr` for some ports requiring further fixes in machine-dependent code of the ports.
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
Iain Sandoe writes:
> Hi Folks.
>> On 14 Feb 2022, at 16:58, Vladimir Makarov wrote:
>> On 2022-02-14 11:00, Richard Sandiford wrote:
>
>>> Vladimir Makarov via Gcc-patches 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, , cn))
>> + if (!valid_address_p (op, , cn) && !flag_pic)
>
> IMO the PIC aspect of this is possibly misleading
> - the issue is that we have an invalid address, and that such addresses in
> this case need to be legitimised by wrapping them in an UNSPEC.
> - My concern about the generic code was that I would not expect Darwin to be
> the only platform that might need to wrap an invlaid address in an unspec
> [TOC, TLS, small data etc. spring to mind].
Yeah, that part is pretty common.
> I need some help understanding the expected pathway through this code that
> could be useful.
>
> we start with an invalid address.
>
> 1. we build (set reg (high invalid_address))
> - Darwin was allowing this (and the lo_sum) [eveywhere, not just here] on
> the basis that the target legitimizer would be called later to fix it up.
> (that is why the initial recog passes) - but AFAICT we never call the
> target’s address legitimizer.
I think it's really the other way around: legitimisers are (or should be)
called before recog, with the legitimisers producing instructions that
recog is known to accept.
Once something has been recog()ed, the target has to be prepared
to generate code for it, either directly from an asm template or
indirectly from a define_split. There's no backing out beyond
that point.
For:
(set … (mem (lo_sum (reg Y) (…
the lo_sum is an address that needs to be legitimised (because it's
in a mem) but in:
(set (reg X) (lo_sum (reg Y) (…)))// A
(set … (mem (reg X)))
it isn't: it's just an ordinary bit of arithmetic whose result happens
to be used as an address later. If the target accepts A then it must be
prepared to generate code for it, without other hooks
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
Hi Folks.
> On 14 Feb 2022, at 16:58, Vladimir Makarov wrote:
> On 2022-02-14 11:00, Richard Sandiford wrote:
>> Vladimir Makarov via Gcc-patches 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, , cn))
> + if (!valid_address_p (op, , cn) && !flag_pic)
IMO the PIC aspect of this is possibly misleading
- the issue is that we have an invalid address, and that such addresses in
this case need to be legitimised by wrapping them in an UNSPEC.
- My concern about the generic code was that I would not expect Darwin to be
the only platform that might need to wrap an invlaid address in an unspec
[TOC, TLS, small data etc. spring to mind].
I need some help understanding the expected pathway through this code that
could be useful.
we start with an invalid address.
1. we build (set reg (high invalid_address))
- Darwin was allowing this (and the lo_sum) [eveywhere, not just here] on the
basis that the target legitimizer would be called later to fix it up. (that is
why the initial recog passes) - but AFAICT we never call the target’s address
legitimizer.
- I am curious about what (other) circumstance there would be where a (high of
an invalid address would be useful.
2. … assuming the we allowed the build of the (high invalid)
- we now build the lo_sum and check to see if it is valid.
3. if it is _not_ valid, we load it into a reg
- I am not sure (outside the comment about about post-legitimiizer use) about
how an invalid lo_sum can be used in this way.
- assuming we accept this, we then test to see if the register is a valid
address (my guess is that test will pass pretty much everywhere, since we
picked a suitable register in the first place).
^^^ this is mostly for my education - the stuff below is a potential solution
to leaving lra-constraints unchanged and fixing the Darwin bug….
[ part of me wonders why we do not just call the target’s address legitimizer
when we have an illegal address ]
——— current WIP:
So .. I have split the Darwin
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
On 2022-02-14 11:00, Richard Sandiford wrote:
Hi Vlad,
Vladimir Makarov via Gcc-patches 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, , cn))
+ if (!valid_address_p (op, , 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, , cn))
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
Iain Sandoe writes:
> Hi Richard,
>
> (hopefully, my take won’t cloud the issue ….)
>
>> On 14 Feb 2022, at 16:00, Richard Sandiford via Gcc-patches
>> wrote:
>>
>> Hi Vlad,
>>
>> Vladimir Makarov via Gcc-patches writes:
>>> On 2022-02-14 04:44, Richard Sandiford via Gcc-patches wrote:
Iain Sandoe via Gcc-patches 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
> Co-authored-by: Vladimir Makarov
>
> 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, , 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, , 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
>>
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
Hi Richard,
(hopefully, my take won’t cloud the issue ….)
> On 14 Feb 2022, at 16:00, Richard Sandiford via Gcc-patches
> wrote:
>
> Hi Vlad,
>
> Vladimir Makarov via Gcc-patches writes:
>> On 2022-02-14 04:44, Richard Sandiford via Gcc-patches wrote:
>>> Iain Sandoe via Gcc-patches 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
Co-authored-by: Vladimir Makarov
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, , 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, , 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.
If X is an invalid address (in this case for
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
Hi Vlad,
Vladimir Makarov via Gcc-patches writes:
> On 2022-02-14 04:44, Richard Sandiford via Gcc-patches wrote:
>> Iain Sandoe via Gcc-patches 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
>>> Co-authored-by: Vladimir Makarov
>>>
>>> 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, , 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, , 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
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
On 2022-02-14 04:44, Richard Sandiford via Gcc-patches wrote:
Iain Sandoe via Gcc-patches 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
Co-authored-by: Vladimir Makarov
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, , 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, , 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.
Re: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
Iain Sandoe via Gcc-patches 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
> Co-authored-by: Vladimir Makarov
>
> 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, , 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, , 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.
Thanks,
Richard
[pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
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
Co-authored-by: Vladimir Makarov
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/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index eaba9a2d698..bc3ef0721a4 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -8317,8 +8317,14 @@ darwin_rs6000_legitimate_lo_sum_const_p (rtx x,
machine_mode mode)
if (GET_CODE (x) == CONST)
x = XEXP (x, 0);
+ /* If we are building PIC code, then any symbol must be wrapped in an
+ UNSPEC_MACHOPIC_OFFSET so that it will get the picbase subtracted. */
+ bool machopic_offs_p = false;
if (GET_CODE (x) == UNSPEC && XINT (x, 1) == UNSPEC_MACHOPIC_OFFSET)
-x = XVECEXP (x, 0, 0);
+{
+ x = XVECEXP (x, 0, 0);
+ machopic_offs_p = true;
+}
rtx sym = NULL_RTX;
unsigned HOST_WIDE_INT offset = 0;
@@ -8349,6 +8355,9 @@ darwin_rs6000_legitimate_lo_sum_const_p (rtx x,
machine_mode mode)
if (sym)
{
tree decl = SYMBOL_REF_DECL (sym);
+ /* As noted above, PIC code cannot use a bare SYMBOL_REF. */
+ if (TARGET_MACHO && flag_pic && !machopic_offs_p)
+ return false;
#if TARGET_MACHO
if (MACHO_SYMBOL_INDIRECTION_P (sym))
/* The decl in an indirection symbol is the original one, which might
@@ -8936,7 +8945,7 @@ legitimate_lo_sum_address_p (machine_mode mode, rtx x,
int strict)
return false;
x = XEXP (x, 1);
- if (TARGET_ELF || TARGET_MACHO)
+ if (TARGET_ELF)
{
bool large_toc_ok;
@@ -8962,7 +8971,32 @@ legitimate_lo_sum_address_p (machine_mode mode, rtx x,
int strict)
return CONSTANT_P (x) || large_toc_ok;
}
+ else if (TARGET_MACHO)
+{
+ if (GET_MODE_NUNITS (mode) != 1)
+ return false;
+ if (GET_MODE_SIZE (mode) > UNITS_PER_WORD
+ && !(/* see above */
+ TARGET_HARD_FLOAT && (mode == DFmode || mode == DDmode)))
+ return false;
+#if TARGET_MACHO
+ if (MACHO_DYNAMIC_NO_PIC_P || !flag_pic)
+ return CONSTANT_P (x);
+#endif
+ /* Macho-O PIC code from here. */
+ if (GET_CODE (x) == CONST)
+ x = XEXP (x, 0);
+
+ /* SYMBOL_REFs need to be wrapped in an UNSPEC_MACHOPIC_OFFSET. */
+ if (SYMBOL_REF_P (x))
+ return false;
+ /* So this is OK if the wrapped object is const. */
+ if (GET_CODE (x) == UNSPEC
+ && XINT (x, 1) == UNSPEC_MACHOPIC_OFFSET)
+ return CONSTANT_P (XVECEXP (x, 0, 0));
+ return CONSTANT_P (x);
+}
return false;
}
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, , 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, , cn))
- {
- *ad.inner = addr;
-
