From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 600873858C78 for ; Thu, 16 Feb 2023 12:06:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 600873858C78 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=linux.ibm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linux.ibm.com Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 31GArONm028728; Thu, 16 Feb 2023 12:06:11 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=pp1; bh=K9EJYKPFtBwmYMHWRhs4KoQ1tSd26aCLlYTT4tnjMsY=; b=FWeBpRkUClpZ8lf7ao7f1ehbuNR79HvsA9JuGplkIMPOjCOm81Zvsu7SONpv6u916RVv fHXmH6D1xZ6FrruNBIcuxyJ4ZyoeXfgZWkgNmy0ZTii49HI8DhQ8FcJtB9Oc4+dOWCQp s9iSlmCQtBk+fn+31yA0+pRMGdOdSvKznBjYQ23EP9i/ja5S2QsALq8dhNTGNYXw4+K8 Z7EmGTlior5UsnpghtpPyhejP4Q6bDFA95Uy2tYM/E3VdTKpqE1o82o68snJCuLyh9au szrVqNVSnC53PVJ8EKktzkRhqGkVOQWUlT4La13kYMfmhVWmynV4McczV2CQywCCtExM 2Q== Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com (PPS) with ESMTPS id 3nsgts53e9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Feb 2023 12:06:11 +0000 Received: from m0098416.ppops.net (m0098416.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 31GBInIC009106; Thu, 16 Feb 2023 12:06:11 GMT Received: from ppma04ams.nl.ibm.com (63.31.33a9.ip4.static.sl-reverse.com [169.51.49.99]) by mx0b-001b2d01.pphosted.com (PPS) with ESMTPS id 3nsgts53da-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Feb 2023 12:06:11 +0000 Received: from pps.filterd (ppma04ams.nl.ibm.com [127.0.0.1]) by ppma04ams.nl.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 31G5Z2Ma010915; Thu, 16 Feb 2023 12:06:09 GMT Received: from smtprelay06.fra02v.mail.ibm.com ([9.218.2.230]) by ppma04ams.nl.ibm.com (PPS) with ESMTPS id 3np2n6xmvc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Feb 2023 12:06:09 +0000 Received: from smtpav01.fra02v.mail.ibm.com (smtpav01.fra02v.mail.ibm.com [10.20.54.100]) by smtprelay06.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 31GC651O22938188 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 16 Feb 2023 12:06:05 GMT Received: from smtpav01.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 9F9322004D; Thu, 16 Feb 2023 12:06:05 +0000 (GMT) Received: from smtpav01.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id CA2F020040; Thu, 16 Feb 2023 12:06:03 +0000 (GMT) Received: from [9.197.240.245] (unknown [9.197.240.245]) by smtpav01.fra02v.mail.ibm.com (Postfix) with ESMTP; Thu, 16 Feb 2023 12:06:03 +0000 (GMT) Message-ID: Date: Thu, 16 Feb 2023 20:06:02 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [PATCH] rs6000: Fix vector parity support [PR108699] Content-Language: en-US To: Segher Boessenkool Cc: GCC Patches , David Edelsohn , Peter Bergner , Michael Meissner References: <20230216111435.GH25951@gate.crashing.org> From: "Kewen.Lin" In-Reply-To: <20230216111435.GH25951@gate.crashing.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: 7JOrvSHTbF881WcF2vcGP9BUTtAaGv0Y X-Proofpoint-ORIG-GUID: ZBrfPJhbN_tEk3MMsQAJVVX0OrL3eZA2 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.170.22 definitions=2023-02-16_09,2023-02-16_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 priorityscore=1501 impostorscore=0 lowpriorityscore=0 adultscore=0 suspectscore=0 mlxlogscore=999 spamscore=0 phishscore=0 malwarescore=0 mlxscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000 definitions=main-2302160103 X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Segher, Thanks for the review comments! on 2023/2/16 19:14, Segher Boessenkool wrote: > Hi! > > On Thu, Feb 16, 2023 at 05:23:40PM +0800, Kewen.Lin wrote: >> This patch is to fix the handling with one more pre-insn >> vpopcntb. It also fixes an oversight having V8HI in VEC_IP, >> replaces VParity with VEC_IP, and adjusts the existing >> UNSPEC_PARITY to a more meaningful name UNSPEC_PARITYB. > > Please don't do that. UNSPEC_PARITYB is worse than UNSPEC_PARITY, > even more so for the prtyw etc. instructions. I thought the scalar insns prty[wd] also operate on byte (especially on the least significant bit in each byte), PARITYB(yte) seems better ... > > You might want to express the vector parity insns separately, but then > *do that*, don't rename the normal stuff as well, and use a more obvious > name like UNSPEC_VPARITY please. I'll update for vector only. Maybe it's better with UNSPEC_VPARITY*B*? since the mnemonic has "b"(yte). > >> const vsll __builtin_altivec_vprtybd (vsll); >> - VPRTYBD parityv2di2 {} >> + VPRTYBD p9v_paritybv2di2 {} > > Why this? Please keep the simpler names if at all possible. The bif would like to map with the vector parity byte insns directly, the parity2 can't work here any more. The name is updated from previous *p9v_parity2 (becoming to a named define_insn), I noticed there are some names with p8v_, p9v_, meant to keep it consistent with the context. You want this to be simplified as parity*b*v2di2? > >> { >> emit_insn (gen_popcntbsi2 (tmp, src)); >> - emit_insn (gen_paritysi2_cmpb (dst, tmp)); >> + emit_insn (gen_paritybsi2 (dst, tmp)); >> } > > It is completely non-obvious what a "paritybsi2" is. There is no such > thing as a "parityb", not for normal people anyway. It is very > important that names give a hint of what they stand for. > > The _cmpb of the existing name indicates that a cmpb insn is generated > here as well. Has that changed> > I got the same understanding initially, but as you may have noticed there isn't a cmpb, it seems just to be different from the name parity2 so put the condition as one suffix. >> -(define_insn "parity2_cmpb" >> +(define_insn "parityb2" >> [(set (match_operand:GPR 0 "gpc_reg_operand" "=r") >> - (unspec:GPR [(match_operand:GPR 1 "gpc_reg_operand" "r")] UNSPEC_PARITY))] >> + (unspec:GPR [(match_operand:GPR 1 "gpc_reg_operand" "r")] >> + UNSPEC_PARITYB))] >> "TARGET_CMPB && TARGET_POPCNTB" >> "prty %0,%1" >> [(set_attr "type" "popcnt")]) > > Hrm, the original name was not so good apparently. Still, please don't > change multiple independent things in one patch, it makes the patch hard > to read and understand and very hard to spot mistakes in. Got it, good point. > >> @@ -1226,7 +1225,16 @@ (define_expand "popcount2" >> (define_expand "parity2" >> [(set (match_operand:VEC_IP 0 "register_operand") >> (parity:VEC_IP (match_operand:VEC_IP 1 "register_operand")))] >> - "TARGET_P9_VECTOR") >> + "TARGET_P9_VECTOR" >> +{ >> + rtx op1 = gen_lowpart (V16QImode, operands[1]); >> + rtx res = gen_reg_rtx (V16QImode); >> + emit_insn (gen_popcountv16qi2 (res, op1)); >> + emit_insn (gen_p9v_parityb2 (operands[0], >> + gen_lowpart (mode, res))); >> + >> + DONE; >> +}) > > So first do a patch that is essentially just this? OK, will update and test it again. > > Later patches can do all other things (also, not do this expand for > TImode at all, ho hum). OK, I guess all the others are for next stage1. :) BR, Kewen