Message ID | 20220210095503.GY2646553@tucnak |
---|---|
State | New |
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 43762385841B for <patchwork@sourceware.org>; Thu, 10 Feb 2022 09:56:30 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 43762385841B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1644486990; bh=/NMYzZqDA4GNvtv/emM+71uB6P7IVZAWaCbOeZQ8o6w=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=V9ZNKCoMQ81nZj2knb9mZ1npSrq9t8jf3TTMF3KKcL6/Y1A7c5MOQD0LxPC3Yp7YM tOH7WAqY4Jca/Z3ZK0tkTRMifdbCk3rJG4gWqeY0+ve5XtPnh3XqPCgUDv+jroLvyI kmcs2DV5RKbYmlTiznEgdHrf0/Vn4WkiBg1TVqcU= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id AF48D385842A for <gcc-patches@gcc.gnu.org>; Thu, 10 Feb 2022 09:55:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AF48D385842A Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-339-FYycFzMmOnqjQkHU_Mp9vw-1; Thu, 10 Feb 2022 04:55:11 -0500 X-MC-Unique: FYycFzMmOnqjQkHU_Mp9vw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1EBFE801ADA; Thu, 10 Feb 2022 09:55:09 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.125]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A82B86C1B3; Thu, 10 Feb 2022 09:55:07 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 21A9t41R530698 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 10 Feb 2022 10:55:05 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 21A9t3HN530697; Thu, 10 Feb 2022 10:55:03 +0100 Date: Thu, 10 Feb 2022 10:55:03 +0100 To: Segher Boessenkool <segher@kernel.crashing.org> Subject: [PATCH] combine: Fix ICE with substitution of CONST_INT into PRE_DEC argument [PR104446] Message-ID: <20220210095503.GY2646553@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jakub Jelinek <jakub@redhat.com> Cc: gcc-patches@gcc.gnu.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 |
combine: Fix ICE with substitution of CONST_INT into PRE_DEC argument [PR104446]
|
|
Commit Message
Jakub Jelinek
Feb. 10, 2022, 9:55 a.m. UTC
Hi! The following testcase ICEs, because combine substitutes (insn 10 9 11 2 (set (reg/v:SI 7 sp [ a ]) (const_int 0 [0])) "pr104446.c":9:5 81 {*movsi_internal} (nil)) (insn 13 11 14 2 (set (mem/f:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A32]) (reg:SI 85)) "pr104446.c":10:3 56 {*pushsi2} (expr_list:REG_DEAD (reg:SI 85) (expr_list:REG_ARGS_SIZE (const_int 16 [0x10]) (nil)))) forming (insn 13 11 14 2 (set (mem/f:SI (pre_dec:SI (const_int 0 [0])) [0 S4 A32]) (reg:SI 85)) "pr104446.c":10:3 56 {*pushsi2} (expr_list:REG_DEAD (reg:SI 85) (expr_list:REG_ARGS_SIZE (const_int 16 [0x10]) (nil)))) which is invalid RTL (pre_dec's argument must be a REG). I know substitution creates various forms of invalid RTL and hopes that invalid RTL just won't recog. But unfortunately in this case we ICE before we get to recog, as try_combine does: if (n_auto_inc) { int new_n_auto_inc = 0; for_each_inc_dec (newpat, count_auto_inc, &new_n_auto_inc); if (n_auto_inc != new_n_auto_inc) { if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, "Number of auto_inc expressions changed\n"); undo_all (); return 0; } } and for_each_inc_dec under the hood will do e.g. for the PRE_DEC case: case PRE_DEC: case POST_DEC: { poly_int64 size = GET_MODE_SIZE (GET_MODE (mem)); rtx r1 = XEXP (x, 0); rtx c = gen_int_mode (-size, GET_MODE (r1)); return fn (mem, x, r1, r1, c, data); } and that code rightfully expects that the PRE_DEC operand has non-VOIDmode (as it needs to be a REG) - gen_int_mode for VOIDmode results in ICE. I think it is better not to emit the clearly invalid RTL during substitution like we do for other cases, than to adding workarounds for invalid IL created by combine to rtlanal.cc and perhaps elsewhere. As for the testcase, of course it is UB at runtime to modify sp that way, but if such code is never reached, we must compile it, not to ICE on it. And I don't see why on other targets which use the autoinc rtxes much more it couldn't happen with other registers. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2022-02-10 Jakub Jelinek <jakub@redhat.com> PR middle-end/104446 * combine.cc (subst): Don't substitute CONST_INTs into RTX_AUTOINC operands. * gcc.target/i386/pr104446.c: New test. Jakub
Comments
Hi! On Thu, Feb 10, 2022 at 10:55:03AM +0100, Jakub Jelinek wrote: > The following testcase ICEs, because combine substitutes > (insn 10 9 11 2 (set (reg/v:SI 7 sp [ a ]) > (const_int 0 [0])) "pr104446.c":9:5 81 {*movsi_internal} > (nil)) > (insn 13 11 14 2 (set (mem/f:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A32]) > (reg:SI 85)) "pr104446.c":10:3 56 {*pushsi2} > (expr_list:REG_DEAD (reg:SI 85) > (expr_list:REG_ARGS_SIZE (const_int 16 [0x10]) > (nil)))) > forming > (insn 13 11 14 2 (set (mem/f:SI (pre_dec:SI (const_int 0 [0])) [0 S4 A32]) > (reg:SI 85)) "pr104446.c":10:3 56 {*pushsi2} > (expr_list:REG_DEAD (reg:SI 85) > (expr_list:REG_ARGS_SIZE (const_int 16 [0x10]) > (nil)))) > which is invalid RTL (pre_dec's argument must be a REG). > I know substitution creates various forms of invalid RTL and hopes that > invalid RTL just won't recog. This is not a "hope"; it is a requirement. If the backend accepts invalid insns, this is a bug in the backend. > But unfortunately in this case we ICE before we get to recog, as > try_combine does: > if (n_auto_inc) > { > int new_n_auto_inc = 0; > for_each_inc_dec (newpat, count_auto_inc, &new_n_auto_inc); > > if (n_auto_inc != new_n_auto_inc) > { > if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, "Number of auto_inc expressions changed\n"); > undo_all (); > return 0; > } > } > and for_each_inc_dec under the hood will do e.g. for the PRE_DEC case: > case PRE_DEC: > case POST_DEC: > { > poly_int64 size = GET_MODE_SIZE (GET_MODE (mem)); > rtx r1 = XEXP (x, 0); > rtx c = gen_int_mode (-size, GET_MODE (r1)); > return fn (mem, x, r1, r1, c, data); > } > and that code rightfully expects that the PRE_DEC operand has non-VOIDmode > (as it needs to be a REG) - gen_int_mode for VOIDmode results in ICE. > I think it is better not to emit the clearly invalid RTL during substitution > like we do for other cases, than to adding workarounds for invalid IL > created by combine to rtlanal.cc and perhaps elsewhere. But we do have that in other cases, and not just for combine. IMO it is a good idea to robustify for_each_inc_dec (simply have it skip if the address is not MODE_INT or such). It also is a good idea to robustify combine subst, just as you do. It is best to do both! > As for the testcase, of course it is UB at runtime to modify sp that way, > but if such code is never reached, we must compile it, not to ICE on it. It is an error at compile time already. The stack pointer is a fixed register. The generic parts of the compiler use it, it is not just a backend thing. There are many more ways to ICE the compiler with register vars, btw. And yes, ICEs are bad of course :-) QoI thing... > And I don't see why on other targets which use the autoinc rtxes much more > it couldn't happen with other registers. Yes. My point (in the PR) was that it is easy enough to make this valid code instead! > PR middle-end/104446 > * combine.cc (subst): Don't substitute CONST_INTs into RTX_AUTOINC > operands. > > * gcc.target/i386/pr104446.c: New test. > +/* PR middle-end/104446 */ > +/* { dg-do compile { target ia32 } } */ > +/* { dg-options "-O2 -mrtd" } */ > + > +register volatile int a __asm__("%esp"); > +void foo (void *); > +void bar (void *); > + > +void > +baz (void) > +{ > + foo (__builtin_return_address (0)); > + a = 0; > + bar (__builtin_return_address (0)); > +} So does it not fail if you make this valid code (by using another register)? bp, si, or di maybe? Okay with that fixed. If fixing it is too hard, okay like this (I don't have to maintain other peoples' backends' testsuites after all...) Thanks! Segher
On Thu, Feb 10, 2022 at 10:10:13AM -0600, Segher Boessenkool wrote: > > case POST_DEC: > > { > > poly_int64 size = GET_MODE_SIZE (GET_MODE (mem)); > > rtx r1 = XEXP (x, 0); > > rtx c = gen_int_mode (-size, GET_MODE (r1)); > > return fn (mem, x, r1, r1, c, data); > > } > > and that code rightfully expects that the PRE_DEC operand has non-VOIDmode > > (as it needs to be a REG) - gen_int_mode for VOIDmode results in ICE. > > I think it is better not to emit the clearly invalid RTL during substitution > > like we do for other cases, than to adding workarounds for invalid IL > > created by combine to rtlanal.cc and perhaps elsewhere. > > But we do have that in other cases, and not just for combine. IMO it > is a good idea to robustify for_each_inc_dec (simply have it skip if the > address is not MODE_INT or such). It also is a good idea to robustify > combine subst, just as you do. It is best to do both! Well, skipping would mean the callback isn't called on it so the autoinc isn't detected. But we could do: rtx c = (GET_MODE (r1) == VOIDmode ? GEN_INT (-size) : gen_int_mode (-size, GET_MODE (r1))); with a comment why do do that. > > PR middle-end/104446 > > * combine.cc (subst): Don't substitute CONST_INTs into RTX_AUTOINC > > operands. > > > > * gcc.target/i386/pr104446.c: New test. > > > +/* PR middle-end/104446 */ > > +/* { dg-do compile { target ia32 } } */ > > +/* { dg-options "-O2 -mrtd" } */ > > + > > +register volatile int a __asm__("%esp"); > > +void foo (void *); > > +void bar (void *); > > + > > +void > > +baz (void) > > +{ > > + foo (__builtin_return_address (0)); > > + a = 0; > > + bar (__builtin_return_address (0)); > > +} > > So does it not fail if you make this valid code (by using another > register)? bp, si, or di maybe? Not on x86, that isn't a general auto-inc-dec target, but uses PRE_DEC etc. only for the sp hard register. For other targets we'd need to somehow convince all the earlier passes (gimple and RTL) not to try to propagate the constant value into the addition inside of a memory address. Jakub
On Thu, Feb 10, 2022 at 05:21:07PM +0100, Jakub Jelinek wrote: > On Thu, Feb 10, 2022 at 10:10:13AM -0600, Segher Boessenkool wrote: > > But we do have that in other cases, and not just for combine. IMO it > > is a good idea to robustify for_each_inc_dec (simply have it skip if the > > address is not MODE_INT or such). It also is a good idea to robustify > > combine subst, just as you do. It is best to do both! > > Well, skipping would mean the callback isn't called on it so the autoinc > isn't detected. Which is fine, because it isn't valid in the first place! The only thing we have to do is not ICE, this RTL is not long for this world. > > So does it not fail if you make this valid code (by using another > > register)? bp, si, or di maybe? > > Not on x86, that isn't a general auto-inc-dec target, but uses PRE_DEC > etc. only for the sp hard register. Ugh. Does it have any benefit from using autoinc at all then? (Actual benefit, not notational convenience). > For other targets we'd need to somehow convince all the earlier passes > (gimple and RTL) not to try to propagate the constant value into the > addition inside of a memory address. I wonder if there is any target for which autoinc is more convenient than inconvenient (other than in inline asm, a whole separate challenge) :-( Segher
On Thu, Feb 10, 2022 at 10:42:03AM -0600, Segher Boessenkool wrote: > > Not on x86, that isn't a general auto-inc-dec target, but uses PRE_DEC > > etc. only for the sp hard register. > > Ugh. Does it have any benefit from using autoinc at all then? (Actual > benefit, not notational convenience). That is the most accurate description of what the push/pop instructions actually do, and the backend has been doing it for decades. Jakub
On Thu, Feb 10, 2022 at 06:23:58PM +0100, Jakub Jelinek wrote: > On Thu, Feb 10, 2022 at 10:42:03AM -0600, Segher Boessenkool wrote: > > > Not on x86, that isn't a general auto-inc-dec target, but uses PRE_DEC > > > etc. only for the sp hard register. > > > > Ugh. Does it have any benefit from using autoinc at all then? (Actual > > benefit, not notational convenience). > > That is the most accurate description of what the push/pop instructions > actually do, and the backend has been doing it for decades. It is exactly as accurate as the simpler direct representation as a parallel (which we have used since 1992). Segher
--- gcc/combine.cc.jj 2022-02-08 20:08:13.821404850 +0100 +++ gcc/combine.cc 2022-02-09 12:19:56.768294280 +0100 @@ -5534,6 +5534,12 @@ subst (rtx x, rtx from, rtx to, int in_d if (!x) return gen_rtx_CLOBBER (VOIDmode, const0_rtx); } + /* CONST_INTs shouldn't be substituted into PRE_DEC, PRE_MODIFY + etc. arguments, otherwise we can ICE before trying to recog + it. See PR104446. */ + else if (CONST_SCALAR_INT_P (new_rtx) + && GET_RTX_CLASS (GET_CODE (x)) == RTX_AUTOINC) + return gen_rtx_CLOBBER (VOIDmode, const0_rtx); else SUBST (XEXP (x, i), new_rtx); } --- gcc/testsuite/gcc.target/i386/pr104446.c.jj 2022-02-09 12:29:14.311505584 +0100 +++ gcc/testsuite/gcc.target/i386/pr104446.c 2022-02-09 12:28:35.329050754 +0100 @@ -0,0 +1,15 @@ +/* PR middle-end/104446 */ +/* { dg-do compile { target ia32 } } */ +/* { dg-options "-O2 -mrtd" } */ + +register volatile int a __asm__("%esp"); +void foo (void *); +void bar (void *); + +void +baz (void) +{ + foo (__builtin_return_address (0)); + a = 0; + bar (__builtin_return_address (0)); +}