Message ID | YkHh2TrgR1+HRG6j@toto.the-meissners.org |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7EB5A385780F for <patchwork@sourceware.org>; Mon, 28 Mar 2022 16:27:40 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7EB5A385780F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1648484860; bh=+8t9N0nMLWvO17iQOL8Aqbs7vydjPopnf9JGSfb30ZQ=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=s2WzwPEDgNoKcoMW6B2/YGIzLxr4uD+Tz41YKq1nq3xlHJTCZJBBuKPnevYVrbIFF 7tu3vHbFA0EoXTCuP6ApYILqzZmpIen2kRWQLbSh7wfGq+Yanl00XDOx2ri+8rDB9u DX/3CA6SKaxlXAXBBq7VDmQa2Xw/cd9vvY8mGahk= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 3660C3858C51 for <gcc-patches@gcc.gnu.org>; Mon, 28 Mar 2022 16:27:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3660C3858C51 Received: from pps.filterd (m0187473.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 22SEan2I020646; Mon, 28 Mar 2022 16:27:10 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3f3epcjhen-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 28 Mar 2022 16:27:10 +0000 Received: from m0187473.ppops.net (m0187473.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 22SGEEEc011825; Mon, 28 Mar 2022 16:27:09 GMT Received: from ppma02dal.us.ibm.com (a.bd.3ea9.ip4.static.sl-reverse.com [169.62.189.10]) by mx0a-001b2d01.pphosted.com with ESMTP id 3f3epcjhee-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 28 Mar 2022 16:27:09 +0000 Received: from pps.filterd (ppma02dal.us.ibm.com [127.0.0.1]) by ppma02dal.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 22SGHNCG004528; Mon, 28 Mar 2022 16:27:08 GMT Received: from b03cxnp07029.gho.boulder.ibm.com (b03cxnp07029.gho.boulder.ibm.com [9.17.130.16]) by ppma02dal.us.ibm.com with ESMTP id 3f1tf9j0rg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 28 Mar 2022 16:27:08 +0000 Received: from b03ledav002.gho.boulder.ibm.com (b03ledav002.gho.boulder.ibm.com [9.17.130.233]) by b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 22SGR76g13173172 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 28 Mar 2022 16:27:07 GMT Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 75C87136061; Mon, 28 Mar 2022 16:27:07 +0000 (GMT) Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id F29A913605D; Mon, 28 Mar 2022 16:27:06 +0000 (GMT) Received: from toto.the-meissners.org (unknown [9.65.244.27]) by b03ledav002.gho.boulder.ibm.com (Postfix) with ESMTPS; Mon, 28 Mar 2022 16:27:06 +0000 (GMT) Date: Mon, 28 Mar 2022 12:27:05 -0400 To: Michael Meissner <meissner@linux.ibm.com>, gcc-patches@gcc.gnu.org, Segher Boessenkool <segher@kernel.crashing.org>, David Edelsohn <dje.gcc@gmail.com>, Peter Bergner <bergner@linux.ibm.com>, Will Schmidt <will_schmidt@vnet.ibm.com> Subject: [PATCH 2/4] Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293 Message-ID: <YkHh2TrgR1+HRG6j@toto.the-meissners.org> Mail-Followup-To: Michael Meissner <meissner@linux.ibm.com>, gcc-patches@gcc.gnu.org, Segher Boessenkool <segher@kernel.crashing.org>, David Edelsohn <dje.gcc@gmail.com>, Peter Bergner <bergner@linux.ibm.com>, Will Schmidt <will_schmidt@vnet.ibm.com> References: <YkHhL5rL39UoKIHC@toto.the-meissners.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <YkHhL5rL39UoKIHC@toto.the-meissners.org> X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: g-VlRQvIxYLqgxJN7Z7lO-hNVX7Cpyqw X-Proofpoint-GUID: zdU8kIv3cp1KNOP6fF8bRGFcp1zCJGxQ X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.850,Hydra:6.0.425,FMLib:17.11.64.514 definitions=2022-03-28_07,2022-03-28_01,2022-02-23_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 lowpriorityscore=0 clxscore=1015 priorityscore=1501 impostorscore=0 spamscore=0 mlxlogscore=999 malwarescore=0 phishscore=0 bulkscore=0 suspectscore=0 adultscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000 definitions=main-2203280090 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_NUMSUBJECT, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Michael Meissner via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Michael Meissner <meissner@linux.ibm.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Optimize vec_splats of vec_extract, PR target/99293
|
|
Commit Message
Michael Meissner
March 28, 2022, 4:27 p.m. UTC
Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293 In looking at PR target/99293, I noticed that the code in the insn vsx_splat_<mode>_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. I have built the spec 2017 benchmark with this patch (#2) and the next patch (#3), along with the first patch in the series for power9 and power10 targets. Most of the floating point benchmarks changed code slightly, due to the scheduling changes that came from changing the insn type attribute. I ran the spec 2017 suite on power9, and I did not not notice any significant changes from these changes. The power9 benchmarks that had code differences with these 2 patches applied in addition to the build with just the first patch applied were: namd_r, pareset_r, povray_r, wrf_r, blender_r, cam4_r, deepsjeng_r, imagick_r, roms_r The power9 benchmarks that had code differences with these 2 patches applied in addition to the build with just the first patch applied were (cactuBSSN_r had changes for power10 but not power9): cactuBSSN_r, namd_r, pareset_r, povray_r, wrf_r, blender_r, cam4_r, deepsjeng_r, imagick_r, nab_r, roms_r I have built bootstrap versions on the following systems. There were no regressions in the runs: Power9 little endian, --with-cpu=power9 Power10 little endian, --with-cpu=power10 Power8 big endian, --with-cpu=power8 (both 32-bit & 64-bit tests) Can I install this into the trunk? After a burn-in period, can I backport and install this into GCC 11 and GCC 10 branches? 2022-03-28 Michael Meissner <meissner@linux.ibm.com> gcc/ PR target/99293 * config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct insn type attribute. Add "p9v" isa attribute for generating the mtvsrdd instruction. --- gcc/config/rs6000/vsx.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Comments
Hi! 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_<mode>_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). Also, there are two insn patterns for mtvsrdd, and you are only touching one here. > * config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct > insn type attribute. Add "p9v" isa attribute for generating the > mtvsrdd instruction. This is in vsx.md, instead. > --- a/gcc/config/rs6000/vsx.md > +++ b/gcc/config/rs6000/vsx.md > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_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). Segher
On Mon, Mar 28, 2022 at 03:28:39PM -0500, Segher Boessenkool wrote: > Hi! > > 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_<mode>_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). 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. > 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_<mode>) and not to this patch (to vsx_splat_<mode>_reg) where there are only two alternatives (the first being xxpermdi and the second being mtvsrdd). > > * config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct > > insn type attribute. Add "p9v" isa attribute for generating the > > mtvsrdd instruction. > > This is in vsx.md, instead. > > > --- a/gcc/config/rs6000/vsx.md > > +++ b/gcc/config/rs6000/vsx.md > > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_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. Off the top of my head, we would need a new "isa" variant (p9v64) that combines p9v and 64-bit. 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.
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_<mode>_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_<mode>) > and not to this patch (to vsx_splat_<mode>_reg) where there are only two > alternatives (the first being xxpermdi and the second being mtvsrdd). I mean vsx_concat_<mode> and vsx_splat_<mode>_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_<mode>_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
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md index ddafbe471dd..ad722cff70f 100644 --- a/gcc/config/rs6000/vsx.md +++ b/gcc/config/rs6000/vsx.md @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg" "@ xxpermdi %x0,%x1,%x1,0 mtvsrdd %x0,%1,%1" - [(set_attr "type" "vecperm,vecmove")]) + [(set_attr "type" "vecperm,mtvsr") + (set_attr "isa" "*,p9v")]) (define_insn "vsx_splat_<mode>_mem" [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa")