[RFA,Bug,target/108892,13,regression] Force re-recognition after changing RTL structure of an insn
Message ID | b5d34537-c54b-ebac-7c7d-89380cf9fe46@ventanamicro.com |
---|---|
State | Committed |
Commit | 4a45f5d6a9b53f7f5446dee47e25b07d413bb7eb |
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 1F694385B510 for <patchwork@sourceware.org>; Wed, 29 Mar 2023 13:48:20 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pl1-x635.google.com (mail-pl1-x635.google.com [IPv6:2607:f8b0:4864:20::635]) by sourceware.org (Postfix) with ESMTPS id 4D5E43858D1E for <gcc-patches@gcc.gnu.org>; Wed, 29 Mar 2023 13:48:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4D5E43858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ventanamicro.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=ventanamicro.com Received: by mail-pl1-x635.google.com with SMTP id u10so14958126plz.7 for <gcc-patches@gcc.gnu.org>; Wed, 29 Mar 2023 06:48:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1680097682; h=subject:cc:to:from:content-language:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=41jSK0/wUM/4xrF4xhgdhIARKA1RoocOYRToASn9fS4=; b=CYAf7pBGOXkStSP7VURh2L6+iAL6FZjRk5gEunO6AYzihagXuh0ze20A/iJG0+SgmZ 14MooHFs52ddlm0QlqbeJ9FU6FNFPk6Hwe+TBDt8927ryydUuyH7hynBRv/GLRwyKFKM Hj5Hat9F/seLZ+sGzkdEiVvBUSV4emvAUh575scdY5f5kxP0zj4x5EA0+U0cUxygRfLs 9SDZCoha98Ilcou6DJRhLRa5t4YQbByf+5xBMozW46qCEZnW/rS4s6k0QfYZwrLBUq2N KmUxOkQknVyfx84Fk1Y6or+GwEtaWOuknYOQcLj3an1vhh+4XEql2CNaUg5e/1eRxyn+ VIlA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680097682; h=subject:cc:to:from:content-language:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=41jSK0/wUM/4xrF4xhgdhIARKA1RoocOYRToASn9fS4=; b=cJc5urBLtEPd265HiGWt6E1zDATNux67OUvnCn00PrC/Atryt5xB0EX84Xyq7o2eBS VEeJPGB095ifZq0mM2OSbvlruz+YBDErkbKRUjZJ3sAh1ECKuJc/LLbqcPkICw3JQrwF JxOvZ3j+fPDKP+SUPs/2+qSvIPE9eejkhEGp6S7VBmifYAzX46rg013Ekm2fl+omN78x nu1U78fDLdadiZ2oh5E3iZuySBAa6eJsEqAmFh8H20BhOyaGtZ6G8csxlw0HG4hdypBd dF98jCUffkVShoVHHIkGNvIZ7W7/sX/s3UqdrTV/u1ie8JBgWxr5++OkrV4R9WR7+1Qw 9Y7A== X-Gm-Message-State: AO0yUKUjgqKH9/Abf7yAp37RjNOclcV+0dIDp58MeEF2p2BLZYZBWT4h LTIfKknoQmMc7786nXguJ/jSTSsZjrSjfqbt8ZS9aA== X-Google-Smtp-Source: AK7set9LWUg2TlGhFh4aGCUuvnfCLblBeM5G48fu/GkLdbCFh5M1YR2CZzA92QEh3aD14BB9cuCH1Q== X-Received: by 2002:a05:6a20:8c14:b0:db:da69:3deb with SMTP id j20-20020a056a208c1400b000dbda693debmr15465335pzh.21.1680097681650; Wed, 29 Mar 2023 06:48:01 -0700 (PDT) Received: from ?IPV6:2601:681:8600:13d0::f0a? ([2601:681:8600:13d0::f0a]) by smtp.gmail.com with ESMTPSA id l21-20020a656815000000b004eecc3080f8sm21829984pgt.29.2023.03.29.06.48.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 29 Mar 2023 06:48:01 -0700 (PDT) Content-Type: multipart/mixed; boundary="------------qbjDMdwmXf188QhZRrLFjiWC" Message-ID: <b5d34537-c54b-ebac-7c7d-89380cf9fe46@ventanamicro.com> Date: Wed, 29 Mar 2023 07:48:00 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Content-Language: en-US From: Jeff Law <jlaw@ventanamicro.com> To: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org> Cc: Segher Boessenkool <segher@kernel.crashing.org> Subject: [RFA][Bug target/108892 ][13 regression] Force re-recognition after changing RTL structure of an insn X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[RFA,Bug,target/108892,13,regression] Force re-recognition after changing RTL structure of an insn
|
|
Commit Message
Jeff Law
March 29, 2023, 1:48 p.m. UTC
So as mentioned in the PR the underlying issue here is combine changes the form of an existing insn, but fails to force re-recognition. As a result other parts of the compiler blow up. > /* Temporarily replace the set's source with the > contents of the REG_EQUAL note. The insn will > be deleted or recognized by try_combine. */ > rtx orig_src = SET_SRC (set); > rtx orig_dest = SET_DEST (set); > if (GET_CODE (SET_DEST (set)) == ZERO_EXTRACT) > SET_DEST (set) = XEXP (SET_DEST (set), 0); > SET_SRC (set) = note; > i2mod = temp; > i2mod_old_rhs = copy_rtx (orig_src); > i2mod_new_rhs = copy_rtx (note); > next = try_combine (insn, i2mod, NULL, NULL, > &new_direct_jump_p, > last_combined_insn); > i2mod = NULL; > if (next) > { > statistics_counter_event (cfun, "insn-with-note combine", 1); > goto retry; > } > SET_SRC (set) = orig_src; > SET_DEST (set) = orig_dest; This code replaces the SET_SRC of an insn in the RTL stream with the contents of a REG_EQUAL note. So given an insn like this: > (insn 122 117 127 2 (set (reg:DI 157 [ _46 ]) > (ior:DI (reg:DI 200) > (reg:DI 251))) "j.c":14:5 -1 > (expr_list:REG_EQUAL (const_int 25769803782 [0x600000006]) > (nil))) It replaces the (ior ...) with a (const_int ...). The resulting insn is passed to try_combine which will try to recognize it, then use it in a combination attempt. Recognition succeeds with the special define_insn_and_split pattern in the risc-v backend resulting in: > (insn 122 117 127 2 (set (reg:DI 157 [ _46 ]) > (const_int 25769803782 [0x600000006])) "j.c":14:5 177 {*mvconst_internal} > (expr_list:REG_EQUAL (const_int 25769803782 [0x600000006]) > (nil))) This is as-expected. Now assume we were unable to combine anything, so try_combine returns NULL_RTX. The quoted code above restores SET_SRC (and SET_DEST) resulting in: > (insn 122 117 127 2 (set (reg:DI 157 [ _46 ]) > (ior:DI (reg:DI 200) > (reg:DI 251))) "j.c":14:5 177 {*mvconst_internal} > (expr_list:REG_EQUAL (const_int 25769803782 [0x600000006]) > (nil))) But this doesn't get re-recognized and we ICE later in LRA. The fix is trivial, reset the INSN_CODE to force re-recognition in the case where try_combine fails. Bootstrapped and regression tested on x86_64 and riscv. OK for the trunk? Jeff gcc/ * combine.cc (combine_instructions): Force re-recognition when potentially changing the underlying RTL structure of an insn. gcc/testsuite/ * gcc.c-torture/compile/pr108892.c: New test
Comments
Hi! On Wed, Mar 29, 2023 at 07:48:00AM -0600, Jeff Law wrote: > So as mentioned in the PR the underlying issue here is combine changes > the form of an existing insn, but fails to force re-recognition. As a > result other parts of the compiler blow up. [snip] > The fix is trivial, reset the INSN_CODE to force re-recognition in the > case where try_combine fails. Thanks for the clear explanation! Okay for trunk. Also okay for all backports (after a week or so on trunk). > * combine.cc (combine_instructions): Force re-recognition when > potentially changing the underlying RTL structure of an insn. When returning the original, might be clearer? Thanks, Segher
On 4/5/23 08:21, Segher Boessenkool wrote: > Hi! > > On Wed, Mar 29, 2023 at 07:48:00AM -0600, Jeff Law wrote: >> So as mentioned in the PR the underlying issue here is combine changes >> the form of an existing insn, but fails to force re-recognition. As a >> result other parts of the compiler blow up. > > [snip] > >> The fix is trivial, reset the INSN_CODE to force re-recognition in the >> case where try_combine fails. > > Thanks for the clear explanation! Okay for trunk. Also okay for all > backports (after a week or so on trunk). Thanks. I haven't seen this on any of the release branches, so no strong opinions on backporting at this time. It's a pretty narrow bug (no surprise given its been latent for something like 10 years). > >> * combine.cc (combine_instructions): Force re-recognition when >> potentially changing the underlying RTL structure of an insn. > > When returning the original, might be clearer? Yea. I'll update the ChangeLog entry. Thanks, Jeff
On Wed, Apr 05, 2023 at 09:07:30AM -0600, Jeff Law wrote: > On 4/5/23 08:21, Segher Boessenkool wrote: > >On Wed, Mar 29, 2023 at 07:48:00AM -0600, Jeff Law wrote: > >>So as mentioned in the PR the underlying issue here is combine changes > >>the form of an existing insn, but fails to force re-recognition. As a > >>result other parts of the compiler blow up. > > > >[snip] > > > >>The fix is trivial, reset the INSN_CODE to force re-recognition in the > >>case where try_combine fails. > > > >Thanks for the clear explanation! Okay for trunk. Also okay for all > >backports (after a week or so on trunk). > Thanks. I haven't seen this on any of the release branches, so no > strong opinions on backporting at this time. It's a pretty narrow bug > (no surprise given its been latent for something like 10 years). Right. But it seems to me it has been there all those years? Does the new testcase fail on older branches? Even if not, it seems clear it is wrong on the older branches as well! But if you think it is too dangerous to backport, let's not. Thanks, Segher
On 4/5/23 11:38, Segher Boessenkool wrote: > On Wed, Apr 05, 2023 at 09:07:30AM -0600, Jeff Law wrote: >> On 4/5/23 08:21, Segher Boessenkool wrote: >>> On Wed, Mar 29, 2023 at 07:48:00AM -0600, Jeff Law wrote: >>>> So as mentioned in the PR the underlying issue here is combine changes >>>> the form of an existing insn, but fails to force re-recognition. As a >>>> result other parts of the compiler blow up. >>> >>> [snip] >>> >>>> The fix is trivial, reset the INSN_CODE to force re-recognition in the >>>> case where try_combine fails. >>> >>> Thanks for the clear explanation! Okay for trunk. Also okay for all >>> backports (after a week or so on trunk). >> Thanks. I haven't seen this on any of the release branches, so no >> strong opinions on backporting at this time. It's a pretty narrow bug >> (no surprise given its been latent for something like 10 years). > > Right. But it seems to me it has been there all those years? Does the > new testcase fail on older branches? Even if not, it seems clear it is > wrong on the older branches as well! I bet if I put the special pattern into an old branch, then ran the testcase it'd probably trigger. > > But if you think it is too dangerous to backport, let's not. It should be crazy safe. Not a tall worried about it being dangerous. More a case of not seeing a lot of value given how narrow the problem is. jeff
Hi again, On Wed, Apr 05, 2023 at 11:43:30AM -0600, Jeff Law wrote: > On 4/5/23 11:38, Segher Boessenkool wrote: > >Right. But it seems to me it has been there all those years? Does the > >new testcase fail on older branches? Even if not, it seems clear it is > >wrong on the older branches as well! > I bet if I put the special pattern into an old branch, then ran the > testcase it'd probably trigger. > > >But if you think it is too dangerous to backport, let's not. > It should be crazy safe. Not a tall worried about it being dangerous. > More a case of not seeing a lot of value given how narrow the problem is. Please just do the usual then? git cherry-pick -x $some_hash on the release branches, and push it if that works flawlessly? And if it doesn't, *that* is a good reason to skip backporting it, sure :-) Segher
On 4/5/23 13:02, Segher Boessenkool wrote: > Hi again, > > On Wed, Apr 05, 2023 at 11:43:30AM -0600, Jeff Law wrote: >> On 4/5/23 11:38, Segher Boessenkool wrote: >>> Right. But it seems to me it has been there all those years? Does the >>> new testcase fail on older branches? Even if not, it seems clear it is >>> wrong on the older branches as well! >> I bet if I put the special pattern into an old branch, then ran the >> testcase it'd probably trigger. >> >>> But if you think it is too dangerous to backport, let's not. >> It should be crazy safe. Not a tall worried about it being dangerous. >> More a case of not seeing a lot of value given how narrow the problem is. > > Please just do the usual then? git cherry-pick -x $some_hash on the > release branches, and push it if that works flawlessly? And if it > doesn't, *that* is a good reason to skip backporting it, sure :-) Well, the usual for something like this is to wait, at least for me. Jeff
diff --git a/gcc/combine.cc b/gcc/combine.cc index 053879500b7..22bf8e1ec89 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -1416,6 +1416,7 @@ combine_instructions (rtx_insn *f, unsigned int nregs) statistics_counter_event (cfun, "insn-with-note combine", 1); goto retry; } + INSN_CODE (temp) = -1; SET_SRC (set) = orig_src; SET_DEST (set) = orig_dest; } diff --git a/gcc/testsuite/gcc.c-torture/compile/pr108892.c b/gcc/testsuite/gcc.c-torture/compile/pr108892.c new file mode 100644 index 00000000000..d7fecd54ecf --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr108892.c @@ -0,0 +1,23 @@ +typedef char __attribute__((__vector_size__ (64))) U; +typedef int __attribute__((__vector_size__ (64))) V; + +int g; +U u; + +static inline __attribute__((__always_inline__)) void +bar (short a, short b, V w) +{ + V v = __builtin_shufflevector ((V) { }, a % (0 != w), 17, 22, 20, 15, + 20, 23, 17, 20, 16, 21, 16, 19, 18, 14, 15, + 14) ^ b; + g *= __builtin_memcmp_eq (0, 0, 2); + v |= 6; + __builtin_ilogb (0); + u = (U) w + (U) v; +} + +void +foo (void) +{ + bar (5, 4, (V){30, 4, 1, 5, 6}); +}