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 4C39A3858C5F for ; Thu, 16 Feb 2023 15:11:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4C39A3858C5F 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 31GFAEmE028146; Thu, 16 Feb 2023 09:10:14 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 31GFAE3E028145; Thu, 16 Feb 2023 09:10:14 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 16 Feb 2023 09:10:13 -0600 From: Segher Boessenkool To: "Kewen.Lin" Cc: GCC Patches , David Edelsohn , Peter Bergner , Michael Meissner Subject: Re: [PATCH] rs6000: Fix vector parity support [PR108699] Message-ID: <20230216151013.GI25951@gate.crashing.org> References: <20230216111435.GH25951@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,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no 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! On Thu, Feb 16, 2023 at 08:06:02PM +0800, Kewen.Lin wrote: > on 2023/2/16 19:14, Segher Boessenkool wrote: > > 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 ... The scalar instruction does not include a "b" in the mnemonic, and it says nothing "byte" or "bit" in the instruction name either. The existing name is simpler, less confusing, simply 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). No, you are right that the semantics are pretty much the same. Please just keep UNSPEC_PARITY everywhere. > >> 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. Ah, because it cannot use the expander here, it has to be a define_insn? Why is that? > 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? Without the "b". But that would be better then, yes. This is a great example why p9v_ in the name is not good: most users do not care at all what ISA version this insn first appeared in. > > 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. Yeah. Something for a future improvement. > >> -(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. And we are in stage 4 so you really really do not want something that may be a mistake, that may cause any problems :-) > > So first do a patch that is essentially just this? > > OK, will update and test it again. Thanks! > > 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. :) Yes exactly. And one (small, self-contained) thing per patch please. Thanks again, Segher