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 1FDAA3858296 for ; Thu, 16 Feb 2023 11:15:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1FDAA3858296 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 31GBEaBQ013427; Thu, 16 Feb 2023 05:14:36 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 31GBEZId013426; Thu, 16 Feb 2023 05:14:35 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 16 Feb 2023 05:14:35 -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: <20230216111435.GH25951@gate.crashing.org> References: 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 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. 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. > const vsll __builtin_altivec_vprtybd (vsll); > - VPRTYBD parityv2di2 {} > + VPRTYBD p9v_paritybv2di2 {} Why this? Please keep the simpler names if at all possible. > { > 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> > -(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. > @@ -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? Later patches can do all other things (also, not do this expand for TImode at all, ho hum). Segher