Message ID | 20220211235904.25765-1-iain@sandoe.co.uk |
---|---|
State | Committed |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D42563858030 for <patchwork@sourceware.org>; Fri, 11 Feb 2022 23:59:49 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D42563858030 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1644623989; bh=w8RV0OG/CEr0xlUAXkXlyAGYM4M3zWkOPc17j3ZbFUE=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=Uw35kTdN4x+hNxMdT89LSfZb/1TNOMwGra38N2fTTHD05BJTE6n6fzRfeN57bRlbL lIkM5Uw88F0Xqm2Zz0MUs8+dj3CWrHV/vNRGaM/mCmaMQJYtnkmGpnOpPuo+TooeFk 9sLDvcgImtOI2CxyxJMptOUasnn/3RWL2/MlVi9I= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) by sourceware.org (Postfix) with ESMTPS id 915EF385841B for <gcc-patches@gcc.gnu.org>; Fri, 11 Feb 2022 23:59:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 915EF385841B Received: by mail-wm1-x336.google.com with SMTP id i19so4945352wmq.5 for <gcc-patches@gcc.gnu.org>; Fri, 11 Feb 2022 15:59:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :mime-version:content-transfer-encoding; bh=w8RV0OG/CEr0xlUAXkXlyAGYM4M3zWkOPc17j3ZbFUE=; b=eB790Xk9gHojbhrHqM++iCKZHKMuvkqfnOUQ9FykTIn87x2rq8N73Yma6BZXKH5Xqx KtrUrK56EAVjcGMBB3GYLbX3m46yveJtpsRt4p5duk73V6oWrkpNLJ9l8n9ckhBZOCrF WuHFy6OorTqNZBYmIIx/hCnozuVg8AicD68YN+zJfWylrTSmN8Ls3E4vfenGPZK4KKlR T9Iyz5BXSesu+TZBxRoR7sn9I8bF4ZwZ83B0ohrPV6kF9cLawKAazRnyPTNa29PPEoCP l6KWK7BaWabqEoSmmPj9Kq3sTqtggofe0QEtBJjumSROeVG60qCBY3+TZjyyEDN4quLw Vg3Q== X-Gm-Message-State: AOAM530KqsLN/wBPTnWvahm1IL5v1QJdK/WeWE2hrGfL8xcwErwaKfzI xagpMjT6vReZs90y0nGXI86YV58AviQ= X-Google-Smtp-Source: ABdhPJxtoH7sZwi85vWyg5Vjhrlh7/oHJ1AQIOeBYfLWPrZJC+09KDGMOHa9T4PMJoNJcAJnl5u8RA== X-Received: by 2002:a7b:c856:: with SMTP id c22mr2196092wml.52.1644623958380; Fri, 11 Feb 2022 15:59:18 -0800 (PST) Received: from localhost.localdomain (host81-138-1-83.in-addr.btopenworld.com. [81.138.1.83]) by smtp.gmail.com with ESMTPSA id bk5sm10700773wrb.10.2022.02.11.15.59.17 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 11 Feb 2022 15:59:17 -0800 (PST) X-Google-Original-From: Iain Sandoe <iain@sandoe.co.uk> To: gcc-patches@gcc.gnu.org Subject: [pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117]. Date: Fri, 11 Feb 2022 23:59:04 +0000 Message-Id: <20220211235904.25765-1-iain@sandoe.co.uk> X-Mailer: git-send-email 2.24.3 (Apple Git-128) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Iain Sandoe via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: iain@sandoe.co.uk Cc: Iain Sandoe <iains.gcc@gmail.com>, segher@kernel.crashing.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[pushed] LRA, rs6000, Darwin: Amend lo_sum use for forced constants [PR104117].
|
|
Commit Message
Iain Sandoe
Feb. 11, 2022, 11:59 p.m. UTC
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(-)
Comments
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. Thanks, Richard
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.
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
Hi Richard, (hopefully, my take won’t cloud the issue ….) > On 14 Feb 2022, at 16:00, Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > 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. If X is an invalid address (in this case for PIC code a SYMBOL_REF is not valid without an UNSPEC wrapper) - then (high X) and (lo_sum (high X) X) are also invalid. AFAICT the target never (this late in the process) gets the opportunity to apply a transform to validate the address. > 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. once again if X is plain invalid - loading it into a register or splitting it does not solve that - we’d need to call the target’s address legitimizer. Of course, if we conclude that it is a target bug, I’ll try to fix it up . thanks Iain > > Thanks, > Richard
Iain Sandoe <iain@sandoe.co.uk> writes: > Hi Richard, > > (hopefully, my take won’t cloud the issue ….) > >> On 14 Feb 2022, at 16:00, Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> >> 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. > > If X is an invalid address (in this case for PIC code a SYMBOL_REF is not > valid without an UNSPEC wrapper) - then (high X) and (lo_sum (high X) X) > are also invalid. AFAICT the target never (this late in the process) gets > the opportunity to apply a transform to validate the address. > >> 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. > > once again if X is plain invalid - loading it into a register or splitting it > does not solve that - we’d need to call the target’s address legitimizer. > > Of course, if we conclude that it is a target bug, I’ll try to fix it up . But if the target treats: (set R2 (high X)) (set R1 (lo_sum R2 X)) as an invalid way of setting R1 to X (e.g. because X is plain invalid), then recog should fail on at least one instruction, preferably both. So yeah, this does feel like a target bug to me. We shouldn't assume that lo_sum/high splits will only be generated by the target. Target-independent code can generate them “spontaneously”, provided that it validates the result. (Combine does this too, for example.) It looks like the LRA code might have been missing a HIGH in the fallback case, but it doesn't sound like that was the bug that was being fixed. Thanks, Richard
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))
Hi Folks. > On 14 Feb 2022, at 16:58, Vladimir Makarov <vmakarov@redhat.com> wrote: > On 2022-02-14 11:00, Richard Sandiford wrote: >> 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) 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 patterns into a hi/lo pair for non-PIC and a hi/lo pair for PIC. I added a predicate for the PIC case that requires it to match (unspec [….] UNSPEC_MACHOPIC_OFFSET). Thus both the attempts (high and (lo_sum will fail recog now which has the same effect as punting on the bad lo_sum (and the original testcase now generates correct code) … … the hardware is slow (well, it isn’t really - faster than a Cray XMP .. but …) .. so regstraps on 11.2 (to check the fix works) and master will take some time. Then, hopefully, there is a target-local fix (and the LRA change could be reverted if that’s the concensus about the best solution) ... cheers Iain
Iain Sandoe <iain@sandoe.co.uk> writes: > Hi Folks. >> On 14 Feb 2022, at 16:58, Vladimir Makarov <vmakarov@redhat.com> wrote: >> On 2022-02-14 11:00, Richard Sandiford wrote: > >>> 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) > > 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 being called first. That distinction also (unfortunately) applies to the canonical form of multiplication by 2^N. In an address (i.e. inside a mem) it's: (mult … (const_int 2^N)) whereas outside of an address (outside of a mem) it's: (lshift … (const_int N)) Also, the address legitimisation hooks are just optimisations. It should always be valid to legitimise an address by moving the address into a register via force_operand instead (at least when can_create_psuedos_p). So the real work of legitimising symbolic references is done by the move patterns. Any legitimisation done by the address hooks is just an optimisation on top of that. > - 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 patterns into a hi/lo pair for non-PIC and a hi/lo pair for PIC. > > I added a predicate for the PIC case that requires it to match (unspec [….] UNSPEC_MACHOPIC_OFFSET). > > Thus both the attempts (high and (lo_sum will fail recog now which has the same effect as punting on the bad lo_sum (and the original testcase now generates correct code) … I don't know the Darwin code, so I can't speak to whether splitting the PIC patterns out makes sense, but: yeah, the end result sounds right to me FWIW. Thanks, Richard > … the hardware is slow (well, it isn’t really - faster than a Cray XMP .. but …) .. so regstraps on 11.2 (to check the fix works) and master will take some time. > > Then, hopefully, there is a target-local fix (and the LRA change could be reverted if that’s the concensus about the best solution) ... > > cheers > Iain
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.
Folks, > On 22 Feb 2022, at 14:44, Vladimir Makarov <vmakarov@redhat.com> 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 <iain@sandoe.co.uk> PR target/PR104117 gcc/ChangeLog: * config/rs6000/darwin.md (@machopic_high_<mode>): New. (@machopic_low_<mode>): 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_<mode>" [(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_<mode>" [(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_<mode>" + [(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_<mode>" + [(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 ATTRIBUTE_UNUSED, else return force_reg (Pmode, x); } - if (SYMBOL_REF_P (x)) + if (SYMBOL_REF_P (x) && !TARGET_MACHO) { enum tls_model model = SYMBOL_REF_TLS_MODEL (x); if (model != 0) diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc index b2c4590153c..90b2c56d245 100644 --- a/gcc/lra-constraints.cc +++ b/gcc/lra-constraints.cc @@ -3625,8 +3625,21 @@ 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)) { - *ad.inner = addr; /* Punt. */ - code = -1; + /* 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; + } + } + } } if (code < 0) --
Hi Iain, On Thu, Feb 24, 2022 at 04:02:30PM +0000, Iain Sandoe wrote: > > On 22 Feb 2022, at 14:44, Vladimir Makarov <vmakarov@redhat.com> 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
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; - code = -1; - } - } - + *ad.inner = addr; /* Punt. */ + code = -1; } } if (code < 0)