From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100077 invoked by alias); 7 Dec 2018 17:31:22 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 99897 invoked by uid 89); 7 Dec 2018 17:31:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Dec 2018 17:31:06 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 421D515AB; Fri, 7 Dec 2018 09:31:05 -0800 (PST) Received: from e120077-lin.cambridge.arm.com (e120077-lin.cambridge.arm.com [10.2.206.231]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 78BBF3F5AF; Fri, 7 Dec 2018 09:31:04 -0800 (PST) Subject: Re: [RFA] [target/87369] Prefer "bit" over "bfxil" To: Jeff Law , gcc-patches , James Greenhalgh References: From: "Richard Earnshaw (lists)" Openpgp: preference=signencrypt Message-ID: Date: Fri, 07 Dec 2018 17:31:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-SW-Source: 2018-12/txt/msg00492.txt.bz2 On 07/12/2018 15:52, Jeff Law wrote: > As I suggested in the BZ, this patch rejects constants with just the > high bit set for the recently added "bfxil" pattern. As a result we'll > return to using "bit" for the test in the BZ. > > I'm not versed enough in aarch64 performance tuning to know if "bit" is > actually a better choice than "bfxil". "bit" results in better code for > the testcase, but that seems more a function of register allocation than > "bit" being inherently better than "bfxil". Obviously someone with > more aarch64 knowledge needs to make a decision here. > > My first iteration of the patch changed "aarch64_high_bits_all_ones_p". > We could still go that way too, though the name probably needs to change. > > I've bootstrapped and regression tested on aarch64-linux-gnu and it > fixes the regression. I've also bootstrapped aarch64_be-linux-gnu, but > haven't done any kind of regression tested on that platform. > > > OK for the trunk? The problem here is that the optimum solution depends on the register classes involved and we don't know this during combine. If we have general register, then we want bfi/bfxil to be used; if we have a vector register, then bit is preferable as it changes 3 inter-bank register copies to a single inter-bank copy; and that copy might be hoisted out of a loop. For example, this case: unsigned long f (unsigned long a, unsigned long b) { return (b & 0x7fffffffffffffff) | (a & 0x8000000000000000); } before your patch this expands to just a single bfxil instruction and that's exactly what we'd want here. With it, however, I'm now seeing f: and x1, x1, 9223372036854775807 and x0, x0, -9223372036854775808 orr x0, x1, x0 ret which seems to be even worse than gcc-8 where we got a bfi instruction. Ultimately, the best solution here will probably depend on which we think is more likely, copysign or the example I give above. It might be that for copysign we'll need to expand initially to some unspec that uses a register initialized with a suitable immediate, but otherwise hides the operation from combine until after that has run, thus preventing the compiler from doing the otherwise right thing. We'd lose in the (hopefully) rare case where the operands really were in general registers, but otherwise win for the more common case where they aren't. R. > > Jeff > > > P > > PR target/87369 > * config/aarch64/aarch64.md (aarch64_bfxil): Do not accept > constant with just the high bit set. That's better handled by > the "bit" pattern. > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 88f66104db3..ad6822410c2 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -5342,9 +5342,11 @@ > (match_operand:GPI 3 "const_int_operand" "n, Ulc")) > (and:GPI (match_operand:GPI 2 "register_operand" "0,r") > (match_operand:GPI 4 "const_int_operand" "Ulc, n"))))] > - "(INTVAL (operands[3]) == ~INTVAL (operands[4])) > - && (aarch64_high_bits_all_ones_p (INTVAL (operands[3])) > - || aarch64_high_bits_all_ones_p (INTVAL (operands[4])))" > + "(INTVAL (operands[3]) == ~INTVAL (operands[4]) > + && ((aarch64_high_bits_all_ones_p (INTVAL (operands[3])) > + && popcount_hwi (INTVAL (operands[3])) != 1) > + || (aarch64_high_bits_all_ones_p (INTVAL (operands[4])) > + && popcount_hwi (INTVAL (operands[4])) != 1)))" > { > switch (which_alternative) > { >