From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1031.google.com (mail-pj1-x1031.google.com [IPv6:2607:f8b0:4864:20::1031]) by sourceware.org (Postfix) with ESMTPS id 0A1D5387701D for ; Tue, 6 Jul 2021 10:16:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0A1D5387701D Received: by mail-pj1-x1031.google.com with SMTP id b14-20020a17090a7aceb029017261c7d206so1183444pjl.5 for ; Tue, 06 Jul 2021 03:16:22 -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=O3XHOXdgiUZvLuSUFl/Wj13d83uBTv+XmUjCtpaEovc=; b=YCZTuVYZffxLLCOXwBbvPiDEzXBjMuYQCQjZENDsk1A9tPGoo43uonD6WIhezy3U8e Udve+nGwkTKaP8kXmcOYqB4prZI12d9j+xEJxNWnYC2f5/NJ8yDQ0K0f0K2JgnEInM3A 15AVR5uAz10RrR4+AiphcW0zluPSMYa3dRHI2PpCzT13P3fClWNOmre2x3GuiW3j3X+s jToxFt9e79vrRzDSX7b3V8NSE61OWYkgfAaZhSisZpiDTqYeOQe1nlrD5N0wQQ0sjA+R EmxvKqANc8MeWrQJqTzYT51IKy/C5Wl2KgQR6P6vAo+GqdI6j/VJ1kqqUyMhlod22ykd K8QQ== X-Gm-Message-State: AOAM530fRXxPm05sWmT95W80dFqXHrwsSqxKjUB9rNiH8aJ2Dy+wJxZR eQdWHEBeJDs0abMCSaCoiWxelde+hX26fhaSwnf4Pg== X-Google-Smtp-Source: ABdhPJyzOY1ifbT2TRZWr6ZAFCriL0tWBSuKQteKZOL6/ynqO0cRB+QYOTR+c2lyhyB0Icpnf4/pS1e9vMY3AWJtLgQ= X-Received: by 2002:a17:90b:8e:: with SMTP id bb14mr16327623pjb.84.1625566582024; Tue, 06 Jul 2021 03:16:22 -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: Christophe Lyon Date: Tue, 6 Jul 2021 12:16:11 +0200 Message-ID: Subject: Re: [ARM] PR98435: Missed optimization in expanding vector constructor To: Kyrylo Tkachov Cc: Prathamesh Kulkarni , gcc Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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: Tue, 06 Jul 2021 10:16:25 -0000 On Tue, 6 Jul 2021 at 11:28, Kyrylo Tkachov via Gcc-patches wrote: > > > > > -----Original Message----- > > From: Prathamesh Kulkarni > > Sent: 06 July 2021 10:25 > > To: Kyrylo Tkachov > > Cc: Christophe LYON ; gcc Patches > patches@gcc.gnu.org> > > Subject: Re: [ARM] PR98435: Missed optimization in expanding vector > > constructor > > > > 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. > Wouldn't testcases with e.g. return (int32x4_t) {a,a,a,a}; exercise the other modes? > Ah, that should be ok then. > Thanks, > Kyrill > > > > > 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