From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 102235 invoked by alias); 14 Feb 2018 18:04:50 -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 102222 invoked by uid 89); 14 Feb 2018 18:04:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=qbc X-HELO: foss.arm.com Received: from usa-sjc-mx-foss1.foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 14 Feb 2018 18:04:47 +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 6261D1435; Wed, 14 Feb 2018 10:04:45 -0800 (PST) Received: from [10.2.207.77] (e100706-lin.cambridge.arm.com [10.2.207.77]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 738ED3F487; Wed, 14 Feb 2018 10:04:44 -0800 (PST) Message-ID: <5A847A3A.9080708@foss.arm.com> Date: Wed, 14 Feb 2018 18:04:00 -0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Jeff Law , Uros Bizjak CC: "gcc-patches@gcc.gnu.org" , richard.sandiford@linaro.org Subject: Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_ patterns accept more zero_extract modes References: <5A7C84A7.9090500@foss.arm.com> <5A7DB51C.20108@foss.arm.com> <605fb4a2-c619-8b01-8c7d-110a8ae38f15@redhat.com> In-Reply-To: <605fb4a2-c619-8b01-8c7d-110a8ae38f15@redhat.com> Content-Type: multipart/mixed; boundary="------------050904040105020505070806" X-SW-Source: 2018-02/txt/msg00859.txt.bz2 This is a multi-part message in MIME format. --------------050904040105020505070806 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-length: 5589 On 13/02/18 16:45, Jeff Law wrote: > On 02/09/2018 07:50 AM, Kyrill Tkachov wrote: >> Hi Uros, >> >> On 08/02/18 22:54, Uros Bizjak wrote: >>> On Thu, Feb 8, 2018 at 6:11 PM, Kyrill Tkachov >>> wrote: >>>> Hi all, >>>> >>>> This patch fixes some fallout in the i386 testsuite that occurs after >>>> the >>>> simplification in patch [1/3] [1]. >>>> The gcc.target/i386/extract-2.c FAILs because it expects to match: >>>> (set (reg:CC 17 flags) >>>> (compare:CC (subreg:QI (zero_extract:SI (reg:HI 98) >>>> (const_int 8 [0x8]) >>>> (const_int 8 [0x8])) 0) >>>> (const_int 4 [0x4]))) >>>> >>>> which is the *cmpqi_ext_2 pattern in i386.md but with the new >>>> simplification >>>> the combine/simplify-rtx >>>> machinery produces: >>>> (set (reg:CC 17 flags) >>>> (compare:CC (subreg:QI (zero_extract:HI (reg:HI 98) >>>> (const_int 8 [0x8]) >>>> (const_int 8 [0x8])) 0) >>>> (const_int 4 [0x4]))) >>>> >>>> Notice that the zero_extract now has HImode like the register source >>>> rather >>>> than SImode. >>>> The existing *cmpqi_ext_ patterns however explicitly demand an >>>> SImode on >>>> the zero_extract. >>>> I'm not overly familiar with the i386 port but I think that's too >>>> restrictive. >>>> The RTL documentation says: >>>> For (zero_extract:m loc size pos) "The mode m is the same as the mode >>>> that >>>> would be used for loc if it were a register." >>>> I'm not sure if that means that the mode of the zero_extract and the >>>> source >>>> register must always match (as is the >>>> case after patch [1/3]) but in any case it shouldn't matter semantically >>>> since we're taking a QImode subreg of the whole >>>> thing anyway. >>>> >>>> So the proposed solution in this patch is to allow HI, SI and DImode >>>> zero_extracts in these patterns as these are the >>>> modes that the ext_register_operand predicate accepts, so that the >>>> patterns >>>> can match the new form above. >>>> >>>> With this patch the aforementioned test passes again and bootstrap and >>>> testing on x86_64-unknown-linux-gnu shows >>>> no regressions. >>>> >>>> Is this ok for trunk if the first patch is accepted? >>> Huh, there are many other zero-extract patterns besides cmpqi_ext_* >>> with QImode subreg of SImode zero_extract in i386.md, used to access >>> high QImode register of HImode pair. A quick grep shows these that >>> have _ext_ in their name: >>> >>> (define_insn "*cmpqi_ext_1" >>> (define_insn "*cmpqi_ext_2" >>> (define_expand "cmpqi_ext_3" >>> (define_insn "*cmpqi_ext_3" >>> (define_insn "*cmpqi_ext_4" >>> (define_insn "addqi_ext_1" >>> (define_insn "*addqi_ext_2" >>> (define_expand "testqi_ext_1_ccno" >>> (define_insn "*testqi_ext_1" >>> (define_insn "*testqi_ext_2" >>> (define_insn_and_split "*testqi_ext_3" >>> (define_insn "andqi_ext_1" >>> (define_insn "*andqi_ext_1_cc" >>> (define_insn "*andqi_ext_2" >>> (define_insn "*qi_ext_1" >>> (define_insn "*qi_ext_2" >>> (define_expand "xorqi_ext_1_cc" >>> (define_insn "*xorqi_ext_1_cc" >>> >>> There are also relevant splitters and peephole2 patterns. >> I see. Another approach I've looked at is removing the mode specifier from >> the zero_extract in these patterns. This means that they can be of any mode >> so they will match all of these modes without creating new patterns through >> iterators. That also works for the testcase and passes bootstrap and >> testing >> however there is the snag that the define_insns that don't start with a "*" >> are used to generate RTL through the gen_* mechanism and in that context >> the absence of a mode on the zero_extract would mean a VOIDmode >> zero_extract >> would be created, which I'm fairly sure is not good. So in my >> experiments I left >> those patterns alone (with an explicit SI on the zero_extract). >> >>> IIRC, SImode zero_extract was enough to catch all high-register uses. >>> There will be a pattern explosion if we want to handle all other >>> integer modes here. However, I'm not a RTL expert, so someone will >>> have to say what is the correct RTX form here. >> Jeff, Richard, could you please give us some guidance on this issue? >> Sorry for the trouble. >> > I don't think any of the patterns above are known to the generic code. > So you just have to check the x86 backend to see their precise uses in a > generator (ie gen_cmpqi_ext_3) and verify those do not allow a VOIDmode > (or any other undesirable mode) to slip through. > > Jeff Thanks Jeff, I did have a look. I think we want to maintain the SImode on the RTL that gets created through these named expanders, as generating a VOIDmode zero_extract is not valid. So my patch leaves those intact. The patch removes the mode from the zero_extract RTXes in patterns that are only ever going to get matched (as opposed to generated). That is the ones that start with "*" in their name. This should allow them to match any of the zero_extract modes that might get generated by the midend. Bootstrapped and tested on x86_64-unknown-linux-gnu. Is this a preferable approach? Thanks, Kyrill 2018-02-14 Kyrylo Tkachov PR target/84164 * config/i386/i386.md (*cmpqi_ext_1, *cmpqi_ext_2, *cmpqi_ext_3, *cmpqi_ext_4, *extzvqi_mem_rex64, *extzvqi, QImode zero_extract peephole, *addqi_ext_2, *testqi_ext_1, *testqi_ext_2, *andqi_ext_1_cc, *andqi_ext_2, *qi_ext_1, *qi_ext_2, *xorqi_ext_1_cc AND QImode subreg of zero_extract peephole): Remove mode from zero_extract. --------------050904040105020505070806 Content-Type: text/x-patch; name="i386-modes.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="i386-modes.patch" Content-length: 6938 commit 7965f92e553ee915c6c8a2bd1b0c20473f732cbb Author: Kyrylo Tkachov Date: Wed Feb 7 15:46:48 2018 +0000 [i386] Remove mode check from zero_extracts within QImode subregs diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index a4832bf..911a73b 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -1333,7 +1333,7 @@ (define_insn "*cmpqi_ext_1" (compare (match_operand:QI 0 "nonimmediate_operand" "QBc,m") (subreg:QI - (zero_extract:SI + (zero_extract (match_operand 1 "ext_register_operand" "Q,Q") (const_int 8) (const_int 8)) 0)))] @@ -1347,7 +1347,7 @@ (define_insn "*cmpqi_ext_2" [(set (reg FLAGS_REG) (compare (subreg:QI - (zero_extract:SI + (zero_extract (match_operand 0 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0) @@ -1372,7 +1372,7 @@ (define_insn "*cmpqi_ext_3" [(set (reg FLAGS_REG) (compare (subreg:QI - (zero_extract:SI + (zero_extract (match_operand 0 "ext_register_operand" "Q,Q") (const_int 8) (const_int 8)) 0) @@ -1387,12 +1387,12 @@ (define_insn "*cmpqi_ext_4" [(set (reg FLAGS_REG) (compare (subreg:QI - (zero_extract:SI + (zero_extract (match_operand 0 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0) (subreg:QI - (zero_extract:SI + (zero_extract (match_operand 1 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0)))] @@ -2931,7 +2931,7 @@ (define_expand "extzv" (define_insn "*extzvqi_mem_rex64" [(set (match_operand:QI 0 "norex_memory_operand" "=Bn") (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "Q") + (zero_extract (match_operand 1 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0))] "TARGET_64BIT && reload_completed" @@ -2952,7 +2952,7 @@ (define_insn "*extzv" (define_insn "*extzvqi" [(set (match_operand:QI 0 "nonimmediate_operand" "=QBc,?R,m") (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "Q,Q,Q") + (zero_extract (match_operand 1 "ext_register_operand" "Q,Q,Q") (const_int 8) (const_int 8)) 0))] "" @@ -2980,7 +2980,7 @@ (define_insn "*extzvqi" (define_peephole2 [(set (match_operand:QI 0 "register_operand") (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand") + (zero_extract (match_operand 1 "ext_register_operand") (const_int 8) (const_int 8)) 0)) (set (match_operand:QI 2 "norex_memory_operand") (match_dup 0))] @@ -6347,11 +6347,11 @@ (define_insn "*addqi_ext_2" (subreg:SI (plus:QI (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "%0") + (zero_extract (match_operand 1 "ext_register_operand" "%0") (const_int 8) (const_int 8)) 0) (subreg:QI - (zero_extract:SI (match_operand 2 "ext_register_operand" "Q") + (zero_extract (match_operand 2 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0)) 0)) (clobber (reg:CC FLAGS_REG))] @@ -8601,7 +8601,7 @@ (define_insn "*testqi_ext_1" (compare (and:QI (subreg:QI - (zero_extract:SI (match_operand 0 "ext_register_operand" "Q,Q") + (zero_extract (match_operand 0 "ext_register_operand" "Q,Q") (const_int 8) (const_int 8)) 0) (match_operand:QI 1 "general_operand" "QnBc,m")) @@ -8617,11 +8617,11 @@ (define_insn "*testqi_ext_2" (compare (and:QI (subreg:QI - (zero_extract:SI (match_operand 0 "ext_register_operand" "Q") + (zero_extract (match_operand 0 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0) (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "Q") + (zero_extract (match_operand 1 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0)) (const_int 0)))] @@ -9152,7 +9152,7 @@ (define_insn "*andqi_ext_1_cc" (compare (and:QI (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0") + (zero_extract (match_operand 1 "ext_register_operand" "0,0") (const_int 8) (const_int 8)) 0) (match_operand:QI 2 "general_operand" "QnBc,m")) @@ -9163,7 +9163,7 @@ (define_insn "*andqi_ext_1_cc" (subreg:SI (and:QI (subreg:QI - (zero_extract:SI (match_dup 1) + (zero_extract (match_dup 1) (const_int 8) (const_int 8)) 0) (match_dup 2)) 0))] @@ -9182,11 +9182,11 @@ (define_insn "*andqi_ext_2" (subreg:SI (and:QI (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "%0") + (zero_extract (match_operand 1 "ext_register_operand" "%0") (const_int 8) (const_int 8)) 0) (subreg:QI - (zero_extract:SI (match_operand 2 "ext_register_operand" "Q") + (zero_extract (match_operand 2 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0)) 0)) (clobber (reg:CC FLAGS_REG))] @@ -9571,7 +9571,7 @@ (define_insn "*qi_ext_1" (subreg:SI (any_or:QI (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0") + (zero_extract (match_operand 1 "ext_register_operand" "0,0") (const_int 8) (const_int 8)) 0) (match_operand:QI 2 "general_operand" "QnBc,m")) 0)) @@ -9591,11 +9591,11 @@ (define_insn "*qi_ext_2" (subreg:SI (any_or:QI (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "%0") + (zero_extract (match_operand 1 "ext_register_operand" "%0") (const_int 8) (const_int 8)) 0) (subreg:QI - (zero_extract:SI (match_operand 2 "ext_register_operand" "Q") + (zero_extract (match_operand 2 "ext_register_operand" "Q") (const_int 8) (const_int 8)) 0)) 0)) (clobber (reg:CC FLAGS_REG))] @@ -9686,7 +9686,7 @@ (define_insn "*xorqi_ext_1_cc" (compare (xor:QI (subreg:QI - (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0") + (zero_extract (match_operand 1 "ext_register_operand" "0,0") (const_int 8) (const_int 8)) 0) (match_operand:QI 2 "general_operand" "QnBc,m")) @@ -9697,7 +9697,7 @@ (define_insn "*xorqi_ext_1_cc" (subreg:SI (xor:QI (subreg:QI - (zero_extract:SI (match_dup 1) + (zero_extract (match_dup 1) (const_int 8) (const_int 8)) 0) (match_dup 2)) 0))] @@ -18800,7 +18800,7 @@ (define_peephole2 (match_operator 1 "compare_operator" [(and:QI (subreg:QI - (zero_extract:SI (match_operand 2 "QIreg_operand") + (zero_extract (match_operand 2 "QIreg_operand") (const_int 8) (const_int 8)) 0) (match_operand 3 "const_int_operand")) --------------050904040105020505070806--