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 ATT
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, &ad, cn)) >> + if (!valid_address_p (op, &ad, 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, &ad, cn)) > + if (!valid_address_p (op, &ad, 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 Darwi
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, &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))
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, &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 wher
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, &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. If X is an invalid address (in this case f
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, &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
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, &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.
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, &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. 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, &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; -