From patchwork Wed Apr 22 16:43:33 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 1275288 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 496mV22qRtz9sSJ for ; Thu, 23 Apr 2020 02:43:52 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 39CD8394B017; Wed, 22 Apr 2020 16:43:50 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 23ADC3948490; Wed, 22 Apr 2020 16:43:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 23ADC3948490 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=Thomas_Schwinge@mentor.com IronPort-SDR: pppgGts/9HGQkjcsA0Smifb4ZE1TNnU3nk34slexRClXmva/OrC3WAGs23IWM37je+W+H/iJjQ YJLKD7RJ69EKws/Jy6Ip5uf3XJoTE5Rsy/gWcEq6lE4ajwOMpDaebXq3w4GoUPgselu9k+DJiR S/lUzsqXqAqJU9rdN6EUOQ9zGfl7oTuWZCQ/6jhaJc3C6rR6SD+Kvkgu4rUvy/2NXiVuF0AH5C ENXnpLQmfzV3KncbdUFUwSY8SjQYv6jC+ccTMxL8gO/tJUDbvD8Zec2by0YtEvRwb1RkGRwuvz 5X8= X-IronPort-AV: E=Sophos;i="5.73,304,1583222400"; d="scan'208,223";a="50139994" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa1.mentor.iphmx.com with ESMTP; 22 Apr 2020 08:43:45 -0800 IronPort-SDR: 9j8rFqqzACt6qqaw6VggANImeF4f4KgX7nS+nWBmpwYiQHV4+4aST/1kpTwQL0eJtYwjK3l8IR LYLH+QMqeo2CvUvi9YzjdmWXedf0WNe15SxsObJZQHbzSxOac8mcHe6kZGBLTEC/WbGa3R8+h5 Xzab01XdvW0aCRuY4AZnyhyhZ8D6H1PQxT8J9ys0DUfTkgVtLLxne3w2EPM8NAU0bYlq7Cgg6m WAtUBhCFOqwTLDAr17Un6kIqsb8w4W7JQslX7z4PA8H7RYidSgZMpgH+i6K/H27LSSy5ATVVE/ g2M= From: Thomas Schwinge To: Tejas Belagod , , , Richard Sandiford , Richard Biener Subject: [rtl] Harden 'set_noop_p' for non-constant selectors [PR94279] (was: [Patch, RTL] Eliminate redundant vec_select moves) In-Reply-To: <529F5318.1030505@arm.com> References: <527A4309.70209@arm.com> <8738n9sj8o.fsf@talisman.default> <527A5EF4.5090505@arm.com> <87y551r01p.fsf@talisman.default> <527A7612.2080406@arm.com> <877gcll9ht.fsf@talisman.default> <527BA073.30900@arm.com> <87zjpg1d5p.fsf@sandifor-thinkpad.stglab.manchester.uk.ibm.com> <527BD411.6060300@arm.com> <878uwwdnx0.fsf@talisman.default> <52962733.7030005@arm.com> <87r4a0c1ul.fsf@talisman.default> <529F5318.1030505@arm.com> User-Agent: Notmuch/0.29.1+93~g67ed7df (https://notmuchmail.org) Emacs/26.1 (x86_64-pc-linux-gnu) Date: Wed, 22 Apr 2020 18:43:33 +0200 Message-ID: <87pnbztqru.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 X-Spam-Status: No, score=-21.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andrew Stubbs , Bill Schmidt Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" Hi! First: please be gentle: I don't speak RTL. ;-) And second: it's been some time. On 2013-12-04T16:06:48+0000, Tejas Belagod wrote: > gcc/ > * rtlanal.c (set_noop_p): Return nonzero in case of redundant vec_select > for overlapping register lanes. This got committed to trunk in r205712. > --- a/gcc/rtlanal.c > +++ b/gcc/rtlanal.c > @@ -1180,6 +1180,26 @@ set_noop_p (const_rtx set) > dst = SUBREG_REG (dst); > } > > + /* It is a NOOP if destination overlaps with selected src vector > + elements. */ > + if (GET_CODE (src) == VEC_SELECT > + && REG_P (XEXP (src, 0)) && REG_P (dst) > + && HARD_REGISTER_P (XEXP (src, 0)) > + && HARD_REGISTER_P (dst)) > + { > + int i; > + rtx par = XEXP (src, 1); > + rtx src0 = XEXP (src, 0); > + int c0 = INTVAL (XVECEXP (par, 0, 0)); > + HOST_WIDE_INT offset = GET_MODE_UNIT_SIZE (GET_MODE (src0)) * c0; > + > + for (i = 1; i < XVECLEN (par, 0); i++) > + if (INTVAL (XVECEXP (par, 0, i)) != c0 + i) > + return 0; > + return simplify_subreg_regno (REGNO (src0), GET_MODE (src0), > + offset, GET_MODE (dst)) == (int)REGNO (dst); > + } > + > return (REG_P (src) && REG_P (dst) > && REGNO (src) == REGNO (dst)); > } In "[amdgcn] internal compiler error: RTL check: expected code 'const_int', have 'reg' in rtx_to_poly_int64, at rtl.h:2379", we recently found that that it's wrong to expect constant selectors, at least in the current code and its usage context. (Thanks, Richard Biener for the guidance!) Not too many actually, but of course, this code has seen some changes since 2013-12-04 (for example, r261530 "Use poly_int rtx accessors instead of hwi accessors"), and also the context may have changed that it's being used in -- so, I'm not sure whether the original code (as quoted above) is actually buggy already, but it already does contain the pattern that 'INTVAL' is used on something without making sure that we're actually dealing with a constant selector. (Has that maybe have been an impossible scenario back then?) Anyway. Attached is a WIP patch "[rtl] Harden 'set_noop_p' for non-constant selectors [PR94279]". Richard Biener said that "A patch like along that line is pre-approved", but given my illiterateness with what I'm deal with here, I'd like that reviewed properly, please. :-) If approving this patch, please respond with "Reviewed-by: NAME " so that your effort will be recorded in the commit log, see . I'll schedule x86_64-pc-linux-gnu and powerpc64le-unknown-linux-gnu bootstrap testing. What other testing does this need? (Asking as this seems to have been added for aarch64, which I'm not set up to test.) So far, I've only confirmed that it does solve the RTL checking issue with libgomp AMD GCN offloading testing. Then, should this also be backported to release branches? GCC 9: same patch as for master branch. GCC 8: pre poly_int, so only need to guard 'INTVAL' (by 'CONST_INT_P', right?). Or, is that not worth it, given that nobody found this to be a problem until now (as far as I know), and/or it's maybe really specific to (or, exposed by) AMD GCN's vector instructions? (For AMD GCN offloading, we only care about master branch.) Grüße Thomas ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter From 3546ac8ef47cf67570834e5a70614907bef40304 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Wed, 22 Apr 2020 16:58:44 +0200 Subject: [PATCH] [rtl] Harden 'set_noop_p' for non-constant selectors [PR94279] --- gcc/rtlanal.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c index c7ab86e228b1..0ebde7622db6 100644 --- a/gcc/rtlanal.c +++ b/gcc/rtlanal.c @@ -1631,12 +1631,18 @@ set_noop_p (const_rtx set) int i; rtx par = XEXP (src, 1); rtx src0 = XEXP (src, 0); - poly_int64 c0 = rtx_to_poly_int64 (XVECEXP (par, 0, 0)); + poly_int64 c0; + if (!poly_int_rtx_p (XVECEXP (par, 0, 0), &c0)) + return 0; poly_int64 offset = GET_MODE_UNIT_SIZE (GET_MODE (src0)) * c0; for (i = 1; i < XVECLEN (par, 0); i++) - if (maybe_ne (rtx_to_poly_int64 (XVECEXP (par, 0, i)), c0 + i)) - return 0; + { + poly_int64 c0i; + if (!poly_int_rtx_p (XVECEXP (par, 0, i), &c0i) + || maybe_ne (c0i, c0 + i)) + return 0; + } return REG_CAN_CHANGE_MODE_P (REGNO (dst), GET_MODE (src0), GET_MODE (dst)) && simplify_subreg_regno (REGNO (src0), GET_MODE (src0), -- 2.25.1