From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) by sourceware.org (Postfix) with ESMTPS id 89BCA3855012 for ; Thu, 5 Aug 2021 12:28:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 89BCA3855012 Received: by mail-ed1-x52c.google.com with SMTP id f13so8047307edq.13 for ; Thu, 05 Aug 2021 05:28:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=LBoxVacFR4MFLJV5sFCV8aJVDMCpPQbHP2kA1A3zvcU=; b=cMweeA6wpoZBid0s8VpDBq9W3wSKs8XdWbvhQYRB0f+hkoQu6xPLSEt/56mz813WHd ketw6tzZRU1tPqjaFNK52jfyAUR0TCrozMysklwQoe4/FlQlMd6ffdO5emTUs/BQCrHm wQzRhrNsvuRJLiUQcoxC/eOAxNZvbzXKt6kRNFVcT9RySHlCf18v1i21sUALXQtRkRvS r5yTUkIMssE2pTEJIgb6l4jmiefx1C7I7jctwc/I7HteAKoDMVSGHwrABdfCe/Q2ovzB zYRzsEAXIRkfPkMFKjF0TYqRVGyDN2hW4ECKyV+ooVBXZoTlS+WvpLsMoa/cok8ve0e0 H/Pg== X-Gm-Message-State: AOAM530Gk5Ib2u+Nhkx+QcK5q/ioGN5DkKCGiypnP82FGTlHoY673AlQ RM8l+JYUVlBzadQYyPTS4eLqNAsbJzv5i1X6zsyD/w== X-Google-Smtp-Source: ABdhPJxX0N+HOPBODiW+sIRwCyhG888ra3uVu9p9eWn9o7Be3Wdr5y4Cu0GgyAOKp7RnM/4f2Mf0YuZzieHkwsi+9jk= X-Received: by 2002:a05:6402:160c:: with SMTP id f12mr6296321edv.122.1628166491439; Thu, 05 Aug 2021 05:28:11 -0700 (PDT) MIME-Version: 1.0 References: <9fd8bc30-f7d1-0171-4147-d570413f7a62@foss.st.com> <1d306b96-daa5-3a47-5e3e-d07ddd56dcf4@foss.st.com> In-Reply-To: From: Prathamesh Kulkarni Date: Thu, 5 Aug 2021 17:57:34 +0530 Message-ID: Subject: Re: [ARM] PR98435: Missed optimization in expanding vector constructor To: Christophe Lyon Cc: Kyrylo Tkachov , gcc Patches Content-Type: multipart/mixed; boundary="0000000000005bc15105c8cf0da8" X-Spam-Status: No, score=-9.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 05 Aug 2021 12:28:15 -0000 --0000000000005bc15105c8cf0da8 Content-Type: text/plain; charset="UTF-8" On Tue, 3 Aug 2021 at 20:52, Christophe Lyon wrote: > > > > On Tue, Aug 3, 2021 at 12:57 PM Prathamesh Kulkarni wrote: >> >> On Tue, 3 Aug 2021 at 14:59, Christophe Lyon >> wrote: >> > >> > >> > >> > On Tue, Jul 6, 2021 at 11:26 AM Prathamesh Kulkarni via Gcc-patches wrote: >> >> >> >> On Tue, 6 Jul 2021 at 13:33, Kyrylo Tkachov wrote: >> >> > >> >> > >> >> > >> >> > > -----Original Message----- >> >> > > From: Prathamesh Kulkarni >> >> > > Sent: 06 July 2021 08:06 >> >> > > To: Christophe LYON >> >> > > Cc: Kyrylo Tkachov ; gcc Patches > >> > > patches@gcc.gnu.org> >> >> > > Subject: Re: [ARM] PR98435: Missed optimization in expanding vector >> >> > > constructor >> >> > > >> >> > > On Thu, 1 Jul 2021 at 16:26, Prathamesh Kulkarni >> >> > > wrote: >> >> > > > >> >> > > > On Wed, 30 Jun 2021 at 20:51, Christophe LYON >> >> > > > wrote: >> >> > > > > >> >> > > > > >> >> > > > > On 29/06/2021 12:46, Prathamesh Kulkarni wrote: >> >> > > > > > On Mon, 28 Jun 2021 at 14:48, Christophe LYON >> >> > > > > > wrote: >> >> > > > > >> >> >> > > > > >> On 28/06/2021 10:40, Kyrylo Tkachov via Gcc-patches wrote: >> >> > > > > >>>> -----Original Message----- >> >> > > > > >>>> From: Prathamesh Kulkarni >> >> > > > > >>>> Sent: 28 June 2021 09:38 >> >> > > > > >>>> To: Kyrylo Tkachov >> >> > > > > >>>> Cc: Christophe Lyon ; gcc Patches >> >> > > > >> > > > > >>>> patches@gcc.gnu.org> >> >> > > > > >>>> Subject: Re: [ARM] PR98435: Missed optimization in expanding >> >> > > vector >> >> > > > > >>>> constructor >> >> > > > > >>>> >> >> > > > > >>>> On Thu, 24 Jun 2021 at 22:01, Kyrylo Tkachov >> >> > > >> >> > > > > >>>> wrote: >> >> > > > > >>>>> >> >> > > > > >>>>>> -----Original Message----- >> >> > > > > >>>>>> From: Prathamesh Kulkarni >> >> > > > > >>>>>> Sent: 14 June 2021 09:02 >> >> > > > > >>>>>> To: Christophe Lyon >> >> > > > > >>>>>> Cc: gcc Patches ; Kyrylo Tkachov >> >> > > > > >>>>>> >> >> > > > > >>>>>> Subject: Re: [ARM] PR98435: Missed optimization in expanding >> >> > > vector >> >> > > > > >>>>>> constructor >> >> > > > > >>>>>> >> >> > > > > >>>>>> On Wed, 9 Jun 2021 at 15:58, Prathamesh Kulkarni >> >> > > > > >>>>>> wrote: >> >> > > > > >>>>>>> On Fri, 4 Jun 2021 at 13:15, Christophe Lyon >> >> > > > > >>>> >> >> > > > > >>>>>> wrote: >> >> > > > > >>>>>>>> On Fri, 4 Jun 2021 at 09:27, Prathamesh Kulkarni via Gcc- >> >> > > patches >> >> > > > > >>>>>>>> wrote: >> >> > > > > >>>>>>>>> Hi, >> >> > > > > >>>>>>>>> As mentioned in PR, for the following test-case: >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>>> #include >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>>> bfloat16x4_t f1 (bfloat16_t a) >> >> > > > > >>>>>>>>> { >> >> > > > > >>>>>>>>> return vdup_n_bf16 (a); >> >> > > > > >>>>>>>>> } >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>>> bfloat16x4_t f2 (bfloat16_t a) >> >> > > > > >>>>>>>>> { >> >> > > > > >>>>>>>>> return (bfloat16x4_t) {a, a, a, a}; >> >> > > > > >>>>>>>>> } >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>>> Compiling with arm-linux-gnueabi -O3 -mfpu=neon -mfloat- >> >> > > > > >>>> abi=softfp >> >> > > > > >>>>>>>>> -march=armv8.2-a+bf16+fp16 results in f2 not being >> >> > > vectorized: >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>>> f1: >> >> > > > > >>>>>>>>> vdup.16 d16, r0 >> >> > > > > >>>>>>>>> vmov r0, r1, d16 @ v4bf >> >> > > > > >>>>>>>>> bx lr >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>>> f2: >> >> > > > > >>>>>>>>> mov r3, r0 @ __bf16 >> >> > > > > >>>>>>>>> adr r1, .L4 >> >> > > > > >>>>>>>>> ldrd r0, [r1] >> >> > > > > >>>>>>>>> mov r2, r3 @ __bf16 >> >> > > > > >>>>>>>>> mov ip, r3 @ __bf16 >> >> > > > > >>>>>>>>> bfi r1, r2, #0, #16 >> >> > > > > >>>>>>>>> bfi r0, ip, #0, #16 >> >> > > > > >>>>>>>>> bfi r1, r3, #16, #16 >> >> > > > > >>>>>>>>> bfi r0, r2, #16, #16 >> >> > > > > >>>>>>>>> bx lr >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>>> This seems to happen because vec_init pattern in neon.md >> >> > > has VDQ >> >> > > > > >>>>>> mode >> >> > > > > >>>>>>>>> iterator, which doesn't include V4BF. In attached patch, I >> >> > > changed >> >> > > > > >>>>>>>>> mode >> >> > > > > >>>>>>>>> to VDQX which seems to work for the test-case, and the >> >> > > compiler >> >> > > > > >>>> now >> >> > > > > >>>>>> generates: >> >> > > > > >>>>>>>>> f2: >> >> > > > > >>>>>>>>> vdup.16 d16, r0 >> >> > > > > >>>>>>>>> vmov r0, r1, d16 @ v4bf >> >> > > > > >>>>>>>>> bx lr >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>>> However, the pattern is also gated on TARGET_HAVE_MVE >> >> > > and I am >> >> > > > > >>>>>> not >> >> > > > > >>>>>>>>> sure if either VDQ or VDQX are correct modes for MVE since >> >> > > MVE >> >> > > > > >>>> has >> >> > > > > >>>>>>>>> only 128-bit vectors ? >> >> > > > > >>>>>>>>> >> >> > > > > >>>>>>>> I think patterns common to both Neon and MVE should be >> >> > > moved to >> >> > > > > >>>>>>>> vec-common.md, I don't know why such patterns were left in >> >> > > > > >>>> neon.md. >> >> > > > > >>>>>>> Since we end up calling neon_expand_vector_init for both >> >> > > NEON and >> >> > > > > >>>> MVE, >> >> > > > > >>>>>>> I am not sure if we should separate the pattern ? >> >> > > > > >>>>>>> Would it make sense to FAIL if the mode size isn't 16 bytes for >> >> > > MVE as >> >> > > > > >>>>>>> in attached patch so >> >> > > > > >>>>>>> it will call neon_expand_vector_init only for 128-bit vectors ? >> >> > > > > >>>>>>> Altho hard-coding 16 in the pattern doesn't seem a good idea to >> >> > > me >> >> > > > > >>>> either. >> >> > > > > >>>>>> ping https://gcc.gnu.org/pipermail/gcc-patches/2021- >> >> > > June/572342.html >> >> > > > > >>>>>> (attaching patch as text). >> >> > > > > >>>>>> >> >> > > > > >>>>> --- a/gcc/config/arm/neon.md >> >> > > > > >>>>> +++ b/gcc/config/arm/neon.md >> >> > > > > >>>>> @@ -459,10 +459,12 @@ >> >> > > > > >>>>> ) >> >> > > > > >>>>> >> >> > > > > >>>>> (define_expand "vec_init" >> >> > > > > >>>>> - [(match_operand:VDQ 0 "s_register_operand") >> >> > > > > >>>>> + [(match_operand:VDQX 0 "s_register_operand") >> >> > > > > >>>>> (match_operand 1 "" "")] >> >> > > > > >>>>> "TARGET_NEON || TARGET_HAVE_MVE" >> >> > > > > >>>>> { >> >> > > > > >>>>> + if (TARGET_HAVE_MVE && GET_MODE_SIZE (GET_MODE >> >> > > > > >>>> (operands[0])) != 16) >> >> > > > > >>>>> + FAIL; >> >> > > > > >>>>> neon_expand_vector_init (operands[0], operands[1]); >> >> > > > > >>>>> DONE; >> >> > > > > >>>>> }) >> >> > > > > >>>>> >> >> > > > > >>>>> I think we should move this to vec-common.md like Christophe >> >> > > said. >> >> > > > > >>>>> Perhaps rather than making it FAIL for non-16 MVE sizes we just >> >> > > disable it in >> >> > > > > >>>> the expander condition? >> >> > > > > >>>>> "TARGET_NEON || (TARGET_HAVE_MVE && GET_MODE_SIZE (< >> >> > > > > >>>> VDQ>mode) != 16)" >> >> > > > > >>>> Is it OK to use mode ? Because using mode resulted >> >> > > in lot >> >> > > > > >>>> of build errors. >> >> > > > > >>>> Also, I think the comparison should be inverted, ie, GET_MODE_SIZE >> >> > > > > >>>> (mode) == 16 since >> >> > > > > >>>> we want to make the pattern pass if target is MVE and vector size is >> >> > > 16 bytes ? >> >> > > > > >>>> Do these changes in attached patch look OK ? >> >> > > > > >>> Yes, you're right. >> >> > > > > >> >> >> > > > > >> Can't this be ARM_HAVE__ARITH like in most expanders in >> >> > > vec-common.md? >> >> > > > > >> >> >> > > > > >> (maybe with a && !TARGET_REALLY_IWMMXT if needed) >> >> > > > > > I wonder if this should be ARM_HAVE__LDST instead since >> >> > > we're >> >> > > > > > initializing the vector ? >> >> > > > > >> >> > > > > >> >> > > > > Well, it really depends on which modes you want to enable. >> >> > > > > >> >> > > > > >> >> > > > > Looks like your move VDQ -> VDQ adds V4BF, V8BF and DI. >> >> > > > > >> >> > > > > Are they all OK for Neon? >> >> > > > > >> >> > > > > They are not OK for MVE. >> >> > > > > >> >> > > > > Ideally you could add testcases to cover to the supported and >> >> > > > > unsupported modes for both Neon and MVE.\ >> >> > > > > >> >> > > > > Before your patch, the expander is enabled for MVE for 64 bit modes >> >> > > > > (V8QI, V4HI, V2SI): what happens in this case? Does the compiler crash >> >> > > > > or is there something else preventing the match? >> >> > > > Hi, >> >> > > > Apparently there is VALID_MVE_MODE macro, so is it better to use: >> >> > > > TARGET_NEON || (TARGET_HAVE_MVE && >> >> > > VALID_MVE_MODE(mode)) >> >> > > > as in the attached patch ? >> >> > >> >> > The change is ok. I would like to see some testcases like Christophe suggested, but this patch just moves the expander around rather than introducing new functionality. >> >> Hi Kyrill, >> >> As mentioned in the first email, the patch improves code-gen for >> >> following test-case: >> >> >> >> bfloat16x4_t f (bfloat16_t a) >> >> { >> >> return (bfloat16x4_t) {a, a, a, a}; >> >> } >> >> >> >> Before patch: >> >> f: >> >> mov r3, r0 @ __bf16 >> >> adr r1, .L4 >> >> ldrd r0, [r1] >> >> mov r2, r3 @ __bf16 >> >> mov ip, r3 @ __bf16 >> >> bfi r1, r2, #0, #16 >> >> bfi r0, ip, #0, #16 >> >> bfi r1, r3, #16, #16 >> >> bfi r0, r2, #16, #16 >> >> bx lr >> >> >> >> After patch: >> >> f: >> >> vdup.16 d16, r0 >> >> vmov r0, r1, d16 @ v4bf >> >> bx lr >> >> >> >> because the patch changes mode from VDQ to VDQX to accommodate bf modes. >> >> I have included the test in the attached patch. >> >> I think Christophe's concerns were mainly about the right modes >> >> getting enabled for MVE. >> >> Unfortunately, I am not sure how to test for that because the FE >> >> catches invalid modes, and we don't >> >> end up hitting the pattern. >> >> >> > >> > Hi Prathamesh, >> > >> > The new testcase fails on arm-linux-gnueabihf: >> > FAIL: gcc.target/arm/simd/pr98435.c (test for excess errors) >> > Excess errors: >> > /aci-gcc-fsf/builds/gcc-fsf-gccsrc/sysroot-arm-none-linux-gnueabihf/usr/include/gnu/stubs.h:7:11: fatal error: gnu/stubs-soft.h: No such file or directory >> > compilation terminated. >> > >> > Because you don't check whether -mfloat-abi=softfp is actually supported. >> > >> > Can you fix that? >> Oops, sorry about that. >> The attached patch fixes the test by requiring arm_softfloat and makes >> it UNSUPPORTED on arm-linux-gnueabihf. >> Does it look OK ? >> > > I don't think that's right: it would make the test unsupported if softfp is not the default even if the toolchain has the needed multilibs. > Did you check eg. with arm-eabi and multilibs enabled? Ah OK, thanks for pointing it out! Does the attached patch look correct ? Thanks, Prathamesh > > Christophe > >> >> Thanks, >> Prathamesh >> > >> > Thanks >> > >> > Christophe >> > >> > >> >> >> >> Thanks, >> >> Prathamesh >> >> > Thanks, >> >> > Kyrill >> >> > >> >> > > ping https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574206.html >> >> > > >> >> > > Thanks, >> >> > > Prathamesh >> >> > > > >> >> > > > Thanks, >> >> > > > Prathamesh >> >> > > > > >> >> > > > > >> >> > > > > Thanks, >> >> > > > > >> >> > > > > >> >> > > > > Christophe >> >> > > > > >> >> > > > > >> >> > > > > > Thanks, >> >> > > > > > Prathamesh >> >> > > > > >> >> >> > > > > >> Christophe >> >> > > > > >> >> >> > > > > >> >> >> > > > > >>> Ok. >> >> > > > > >>> Thanks, >> >> > > > > >>> Kyrill >> >> > > > > >>> >> >> > > > > >>> >> >> > > > > >>>> Thanks, >> >> > > > > >>>> Prathamesh >> >> > > > > >>>>> Thanks, >> >> > > > > >>>>> Kyrill >> >> > > > > >>>>> >> >> > > > > >>>>>> Thanks, >> >> > > > > >>>>>> Prathamesh >> >> > > > > >>>>>>> Thanks, >> >> > > > > >>>>>>> Prathamesh >> >> > > > > >>>>>>>> That being said, I suggest you look at other similar patterns in >> >> > > > > >>>>>>>> vec-common.md, most of which are gated on >> >> > > > > >>>>>>>> ARM_HAVE__ARITH >> >> > > > > >>>>>>>> and possibly beware of issues with iwmmxt :-) >> >> > > > > >>>>>>>> >> >> > > > > >>>>>>>> Christophe >> >> > > > > >>>>>>>> >> >> > > > > >>>>>>>>> Thanks, >> >> > > > > >>>>>>>>> Prathamesh --0000000000005bc15105c8cf0da8 Content-Type: text/plain; charset="US-ASCII"; name="pr98435-test-fix-2.txt" Content-Disposition: attachment; filename="pr98435-test-fix-2.txt" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_kryw8fy60 ZGlmZiAtLWdpdCBhL2djYy90ZXN0c3VpdGUvZ2NjLnRhcmdldC9hcm0vc2ltZC9wcjk4NDM1LmMg Yi9nY2MvdGVzdHN1aXRlL2djYy50YXJnZXQvYXJtL3NpbWQvcHI5ODQzNS5jCmluZGV4IDBhZjg2 MzNmZDU2Li5iN2JhNTExZTJkOSAxMDA2NDQKLS0tIGEvZ2NjL3Rlc3RzdWl0ZS9nY2MudGFyZ2V0 L2FybS9zaW1kL3ByOTg0MzUuYworKysgYi9nY2MvdGVzdHN1aXRlL2djYy50YXJnZXQvYXJtL3Np bWQvcHI5ODQzNS5jCkBAIC0zLDYgKzMsNyBAQAogLyogeyBkZy1yZXF1aXJlLWVmZmVjdGl2ZS10 YXJnZXQgYXJtX3Y4XzJhX2JmMTZfbmVvbl9vayB9ICovCiAvKiB7IGRnLWFkZC1vcHRpb25zIGFy bV92OF8yYV9iZjE2X25lb24gfSAqLwogLyogeyBkZy1hZGRpdGlvbmFsLW9wdGlvbnMgIi1tZmxv YXQtYWJpPXNvZnRmcCAtbWFyY2g9YXJtdjguMi1hK2JmMTYrZnAxNiIgfSAqLworLyogeyBkZy1z a2lwLWlmICJza2lwIHRlc3QgZm9yIGhhcmQgZmxvYXQiIHsgKi0qLSogfSB7ICItbWZsb2F0LWFi aT1oYXJkIiB9IHsgIiIgfSB9ICovCiAKICNpbmNsdWRlIDxhcm1fbmVvbi5oPgogCg== --0000000000005bc15105c8cf0da8--