From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 62862 invoked by alias); 15 Feb 2018 07:25: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 62830 invoked by uid 89); 15 Feb 2018 07:25:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-io0-f195.google.com Received: from mail-io0-f195.google.com (HELO mail-io0-f195.google.com) (209.85.223.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Feb 2018 07:25:20 +0000 Received: by mail-io0-f195.google.com with SMTP id n7so27910768iob.0 for ; Wed, 14 Feb 2018 23:25:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=w6P3S8RIHSNcuEu28Mz74PzQ99CbaO7Y/wnRrJPvQGI=; b=metPNbdYn3hKK2llQ33WwHbrbfnHyAYW0DJP9bRTd63WSHqMAOh3z08FP6DemWqBse HaZe6Lv2RknSgiHfS5xAped/Sup/vmt4pwNZnYXlN0m3cT49d/+nUZLU/jjOey6+/Ap4 H7+baoY2ow3EXQh1ugJAu32gvaeDrsW4cmaTuXijH9pUeWQ5SEeSkpxPnVYf14gGAvVa iTVidLjPlvHZMQTLWQ6NbKhAnj3mAtVYbKrTRI+lhTUwYiRqLSTtu3R3txXUipnoXhoM CocMdBVnXMHVRTivYD/NWDGdhe/2fnT3aGFlgBN8dfSU1Whl6uawmlhM41CUTu4GV/cA kBYA== X-Gm-Message-State: APf1xPBaIw1mTLysDp9LgBk/Uc4BLajLKjYokBlNzHiwtlc5AxftIRqa lGnwdcrI39QLhR1v9QtaRPnR1R0M9Ix4LE/DsEM= X-Google-Smtp-Source: AH8x225a0ih0+s3k79yGpmDC/CU1s3ho1TVmtFvCMHxaHW0+iixkslC8kcKk2P2aWpjX5RSuepS0v2FeubnrVzEmCQo= X-Received: by 10.107.138.139 with SMTP id c11mr2351988ioj.280.1518679518301; Wed, 14 Feb 2018 23:25:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.2.21.21 with HTTP; Wed, 14 Feb 2018 23:25:17 -0800 (PST) In-Reply-To: <5A847A3A.9080708@foss.arm.com> References: <5A7C84A7.9090500@foss.arm.com> <5A7DB51C.20108@foss.arm.com> <605fb4a2-c619-8b01-8c7d-110a8ae38f15@redhat.com> <5A847A3A.9080708@foss.arm.com> From: Uros Bizjak Date: Thu, 15 Feb 2018 07:25:00 -0000 Message-ID: Subject: Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_ patterns accept more zero_extract modes To: Kyrill Tkachov Cc: Jeff Law , "gcc-patches@gcc.gnu.org" , richard.sandiford@linaro.org Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2018-02/txt/msg00893.txt.bz2 On Wed, Feb 14, 2018 at 7:04 PM, Kyrill Tkachov wrote: > > 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? We are trying to avoid VOIDmode RTXes as much as possible (to tighten pattern matching to avoid various surpsrises). Looking at these instructions, I think that using SWI248 in insn and peephole2 patterns should be OK. So, if it survives regression tests, the patch is OK with SWI248 mode instead of void mode. Thanks, Uros. > > 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. >