From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id A1ACF3857C71 for ; Fri, 1 Apr 2022 17:22:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A1ACF3857C71 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 231HLhEi015900; Fri, 1 Apr 2022 12:21:43 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 231HLh9t015899; Fri, 1 Apr 2022 12:21:43 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 1 Apr 2022 12:21:43 -0500 From: Segher Boessenkool To: Michael Meissner , gcc-patches@gcc.gnu.org, David Edelsohn , Peter Bergner , Will Schmidt Subject: Re: [PATCH 2/4] Make vsx_splat__reg use correct insn attributes, PR target/99293 Message-ID: <20220401172143.GF614@gate.crashing.org> References: <20220328202839.GN614@gate.crashing.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_NUMSUBJECT, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no 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: Fri, 01 Apr 2022 17:22:47 -0000 On Wed, Mar 30, 2022 at 06:41:59PM -0400, Michael Meissner wrote: > On Mon, Mar 28, 2022 at 03:28:39PM -0500, Segher Boessenkool wrote: > > On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote: > > > In looking at PR target/99293, I noticed that the code in the insn > > > vsx_splat__reg used "vecmove" as the "type" insn attribute when the > > > "mtvsrdd" is generated. It should use "mfvsr". I also added a "p9v" isa > > > attribute for that alternative. > > > > s/mfvsr/mtvsr/ > > > > But, mtvsrd and mtvsrdd have very different scheduling properties (like, > > on p10 it is 1 cycle vs. 3 cycles). > > I must admit, I assumed vecmove was a stand-in for XXMR (i.e. XXLOR). That is "veclogical". I don't think there is any core where this is optimised specially? > Since > its use is used for other cases (mtvsrdd, xxsel/vsel, x{s,v}abs*, x{s,v}nabs*, > xsiexpq*), it is probably better to just let things lie, and perhaps relook at > it in the GCC 13 time frame. Yes, we need to make better categories. The problem is to come up with something that is close enough to what the relevant cores actually do, but in such a way that we do not end up with gazillions of nonsensical separate instruction types. What we care about most for p9 and p10 vector insns is whether something is a 3-cycle op or not. But this differs per core, and in ways that are a little ad-hoc (looked at from far away anyway). For the integer insns we ended up with extra attributes (not just "type"), which is both compact and expressive. We should try to do something like that for vector ops as well. We now have both p9 an p10, with two implementations it should be clearer what a good direction to take will be here. > > Also, there are two insn patterns for mtvsrdd, and you are only touching > > one here. > > I think you meant that comment about the third patch (to vsx_extract_) > and not to this patch (to vsx_splat__reg) where there are only two > alternatives (the first being xxpermdi and the second being mtvsrdd). I mean vsx_concat_ and vsx_splat__reg. Both have mtvsrdd (both as alternative 1), but you only update the "type" of the latter here. > > > --- a/gcc/config/rs6000/vsx.md > > > +++ b/gcc/config/rs6000/vsx.md > > > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat__reg" > > > "@ > > > xxpermdi %x0,%x1,%x1,0 > > > mtvsrdd %x0,%1,%1" > > > - [(set_attr "type" "vecperm,vecmove")]) > > > + [(set_attr "type" "vecperm,mtvsr") > > > + (set_attr "isa" "*,p9v")]) > > > > "we" requires "p9v". Please do a full conversion when getting rid of > > this? That includes requiring TARGET_POWERPC64 for it (not -m64 as its > > documentation says; the existing implementation of "we" is correct). > > That is more complex, and likely it should be a GCC 13 thing. Yes. > Off the top of > my head, we would need a new "isa" variant (p9v64) that combines p9v and > 64-bit. Not at all no. Things that *use* the "isa" attribute can use other attributes as well, if they want. The reason we have "p9v" is because it is so common that a shorthand helps (and *all* p9 vector insns need either it or separate stuff). > Originally, I had changed the "we" to "wa", but then I realized it > wouldn't work for 32-bit, but I left in setting the alternative. Yeah, when I got rid of many of the w* things I left mostly the harder ones for later. Sorry! Segher