From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com [IPv6:2607:f8b0:4864:20::f29]) by sourceware.org (Postfix) with ESMTPS id 42B903856080 for ; Thu, 30 Jun 2022 08:53:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 42B903856080 Received: by mail-qv1-xf29.google.com with SMTP id u7so6299235qvm.4 for ; Thu, 30 Jun 2022 01:53:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vUV7iqPBxQRyhkUr/yzQxrqt4TLv7AXQ8ckIpUWaEZs=; b=zaBu2jlyVE+3k9hH4wFyIuKNB0Xys6N/0A02JTDt8OJZb3wcvUMmwyM+hXj8jHXN0z cvbT6Jl8z8J5SEuqla8BlVE0yq5hjRL4boruIYmVbp353aT9RvAFNfjQQFrbh3A5s3QC frxpBW4rYWFnG6bBgz7gZFyLwbb/8AYR0+nXfkKUbY/RskNLiZbEseTa4MMLFw6A6w9f CJPs7zflkQV749NsWUEJnupL6ggNMiFIbSTkxfiWHY2/geoOYC26DQpEFfAweqBDl/QD 4cJwxa8E9Vpl45Pxs1LnQFtPQXZlArilQDSVoJQV4hxhcp6FBaOo/2PjKEVZuXBNv+KP mLaA== X-Gm-Message-State: AJIora+tDXZ819b5fJrXIPS4qKeX80mX54CHzdTgk8yujxS7rg6RW8lZ R7/RFdjcN60phGNroVitiatof5sMHD4pO9vkV1+iNgw8RmJMeA== X-Google-Smtp-Source: AGRyM1sWMzhVbDbiLRnPG1OlWnLGahNIIMczJp5xQ9ehiYG3XJHJQV2Y8/OKgkglwG+5PugAWAd9SkaOXhWBuvJVj+k= X-Received: by 2002:a05:6214:c83:b0:470:b3e3:c25e with SMTP id r3-20020a0562140c8300b00470b3e3c25emr11257253qvr.1.1656579203615; Thu, 30 Jun 2022 01:53:23 -0700 (PDT) MIME-Version: 1.0 References: <20220630055907.50030-1-haochen.jiang@intel.com> In-Reply-To: From: Uros Bizjak Date: Thu, 30 Jun 2022 10:53:12 +0200 Message-ID: Subject: Re: [PATCH] i386: Extend cvtps2pd to memory To: "Jiang, Haochen" Cc: "gcc-patches@gcc.gnu.org" , "Liu, Hongtao" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Thu, 30 Jun 2022 08:53:25 -0000 On Thu, Jun 30, 2022 at 10:45 AM Uros Bizjak wrote: > > On Thu, Jun 30, 2022 at 9:41 AM Uros Bizjak wrote: > > > > On Thu, Jun 30, 2022 at 9:24 AM Jiang, Haochen wrote: > > > > > > > -----Original Message----- > > > > From: Uros Bizjak > > > > Sent: Thursday, June 30, 2022 2:20 PM > > > > To: Jiang, Haochen > > > > Cc: gcc-patches@gcc.gnu.org; Liu, Hongtao > > > > Subject: Re: [PATCH] i386: Extend cvtps2pd to memory > > > > > > > > On Thu, Jun 30, 2022 at 7:59 AM Haochen Jiang > > > > wrote: > > > > > > > > > > Hi all, > > > > > > > > > > This patch aims to fix the cvtps2pd insn, which should also work on > > > > > memory operand but currently does not. After this fix, when loop == 2, > > > > > it will eliminate movq instruction. > > > > > > > > > > Regtested on x86_64-pc-linux-gnu. Ok for trunk? > > > > > > > > > > BRs, > > > > > Haochen > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > PR target/43618 > > > > > * config/i386/sse.md (extendv2sfv2df2): New define_expand. > > > > > (sse2_cvtps2pd_load): Rename extendvsdfv2df2. > > Rename FROM ... > > Please also mention change to sse2_cvtps2pd. > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > PR target/43618 > > > > > * gcc.target/i386/pr43618-1.c: New test. > > > > > > > > This patch could be as simple as: > > > > > > > > diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md index > > > > 8cd0f617bf3..c331445cb2d 100644 > > > > --- a/gcc/config/i386/sse.md > > > > +++ b/gcc/config/i386/sse.md > > > > @@ -9195,7 +9195,7 @@ > > > > (define_insn "extendv2sfv2df2" > > > > [(set (match_operand:V2DF 0 "register_operand" "=v") > > > > (float_extend:V2DF > > > > - (match_operand:V2SF 1 "register_operand" "v")))] > > > > + (match_operand:V2SF 1 "nonimmediate_operand" "vm")))] > > > > "TARGET_MMX_WITH_SSE" > > > > "%vcvtps2pd\t{%1, %0|%0, %1}" > > > > [(set_attr "type" "ssecvt") > > > > > > We also tested on this version, it is ok. > > > > > > The reason why the patch looks like this is because in the previous insn > > > sse2_cvtps2pd, the constraint vm and vector_operand > > > actually does not match the actual instruction. Memory operand is V2SF, > > > not V4SF. > > > > > > Therefore, we changed the constraint in that insn. Then it caused another issue. > > > For memory operand, it seems that we cannot generate those mask instructions. > > > So I change the pattern to how extendv2hfv2df2 works. > > > > If you want to change the memory access in sse2_cvtps2pd, > > then please see how e.g. v2hiv2di is handled in sse.md. In > > addition to two instructions, you will need one define_insn_and_split > > with a pre-reload splitter. > > Oh, nowadays combine does vec_select from a paradoxical subreg on its own. > > +(define_expand "extendv2sfv2df2" > + [(set (match_operand:V2DF 0 "register_operand") > + (float_extend:V2DF > + (match_operand:V2SF 1 "nonimmediate_operand")))] > + "TARGET_MMX_WITH_SSE" > +{ > + if (!MEM_P (operands[1])) > + { > > You will need force reg here: > > rtx op1 = force_reg (V2SFmode, operands[1]); > + operands[1] = lowpart_subreg (V4SFmode, op1, V2SFmode); > + emit_insn (gen_sse2_cvtps2pd (operands[0], operands[1])); > + DONE; > + } > +}) > > > -(define_insn "extendv2sfv2df2" > +(define_insn "sse2_cvtps2pd_load" > > Please name this insn "*sse2_cvtps2pd_1". Please note the > star at the beginning, You don't have to make the name public. > > OK with the above changes. Forgot to mention: - (match_operand:V2SF 1 "register_operand" "v")))] - "TARGET_MMX_WITH_SSE" - "%vcvtps2pd\t{%1, %0|%0, %1}" + (match_operand:V2SF 1 "memory_operand" "m")))] + "TARGET_MMX_WITH_SSE && " + "%vcvtps2pd\t{%1, %0|%0, %q1}" [(set_attr "type" "ssecvt") The new insn does not need to be limited to TARGET_MMX_WITH_SSE, so we can use TARGET_SSE2 here. Which opens the question if the expander could also be TARGET_SSE2 only. There are no MMX registers involved in any of the patterns anymore. Uros. > > Thanks, > Uros,