From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id CC76B3857C65 for ; Fri, 9 Oct 2020 23:03:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CC76B3857C65 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=segher@kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 099N2cQU016593; Fri, 9 Oct 2020 18:02:38 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 099N2ccI016592; Fri, 9 Oct 2020 18:02:38 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 9 Oct 2020 18:02:38 -0500 From: Segher Boessenkool To: Alex Coplan Cc: gcc-patches@gcc.gnu.org Subject: Re: [PING][PATCH v2] combine: Don't turn (mult (extend x) 2^n) into extract [PR96998] Message-ID: <20201009230238.GL2672@gate.crashing.org> References: <20200930103931.raxfvhpuehrgulcp@arm.com> <20201008102125.vh7qnmpr5alhp5sd@arm.com> <20201008202022.GJ2672@gate.crashing.org> <20201009083808.ajx6nudkqipeexmo@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201009083808.ajx6nudkqipeexmo@arm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-7.0 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_SHORT, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=no 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: , X-List-Received-Date: Fri, 09 Oct 2020 23:03:43 -0000 On Fri, Oct 09, 2020 at 09:38:09AM +0100, Alex Coplan wrote: > Hi Segher, > > On 08/10/2020 15:20, Segher Boessenkool wrote: > > On Thu, Oct 08, 2020 at 11:21:26AM +0100, Alex Coplan wrote: > > > Ping. The kernel is still broken on AArch64. > > > > You *cannot* fix a correctness bug with a combine addition. > > https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555158.html > explains why we do precisely that. And it still is wrong. > Also, as your own testing confirmed, the patch does indeed fix the issue. No, it did not. It showed that before the patch the bug was hit, and after it it was not. It does not show the bug was solved. > > So please fix the target bug first. > > I think the problem here -- the reason that we're talking past each > other -- is that there are (at least) two parts of the codebase that can > be blamed for the ICE here: > > 1. aarch64: "The insn is unrecognized, so it's a backend bug > (missing pattern or similar)." > > 2. combine: "Combine produces a non-canonical insn, so the backend > (correctly) doesn't recognise it, and combine is at fault." That cannot cause ICEs! *That* is the issue. It is *normal* for insns combine comes up with to not be allowed by the backend (or recognised even); in that case, that instruction combination is simply not made. The code was valid before, and stays valid. That is all that combine does: it may or may not change some instructions to semantically equivalent instructions that the cost model thinks are better to have. You *cannot* depend on combine to make *any* particular combination or simplification or anything. There are hundreds of reasons to abort a combination. 3. The insn is not valid for the target, so the target does *not* recognise it, and combine does *not* make that combination, *and all is good*. That is how it is supposed to work (and how it *does* work). If the input to combine contains invalid instructions, you have to fix a bug in some earlier pass (or the backend). All instructions in the instruction stream have to be valid (some time shortly after expand, and passes can do crazy stuff internally, so let's say "between passes"). > Now I initially (naively) took interpretation 1 here and tried to fix > the ICE by adding a pattern to recognise the sign_extract insn that > combine is producing here: > https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553605.html > > Howerver, in the review of that patch, Richard opened my eyes to > interpretation 2, which in hindsight is clearly a better way to fix the > issue. > > Combine already does the canonicalisation for the (ashift x n) case, so > it seems like an obvious improvement to do the same for the (mult x 2^n) > case, as this is how shifts are represented inside mem rtxes. An improvement perhaps, but there is still a bug in the backend. It is *normal* for combine to create code invalid for the target, including non-canonical code. You should not recognise non-canonical code if you cannot actually handle it correctly. > Again, please see Richard's comments here: > https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554518.html > > > > > I haven't had time to look at your patch yet, sorry. > > Not to worry. Hopefully this clears up any confusion around what we're > trying to do here and why. It doesn't, unfortunately. But maybe you understand now? Segher