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 7B055385E447; Tue, 8 Jun 2021 23:26:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7B055385E447 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail 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 158NPiGj018485; Tue, 8 Jun 2021 18:25:44 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 158NPiLA018484; Tue, 8 Jun 2021 18:25:44 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 8 Jun 2021 18:25:43 -0500 From: Segher Boessenkool To: Xionghu Luo Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com, wschmidt@linux.ibm.com, guojiufu@linux.ibm.com, linkw@gcc.gnu.org Subject: Re: [PATCH] rs6000: Remove unspecs for vec_mrghl[bhw] Message-ID: <20210608232543.GC18427@gate.crashing.org> References: <20210524090213.2813103-1-luoxhu@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210524090213.2813103-1-luoxhu@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Tue, 08 Jun 2021 23:26:47 -0000 On Mon, May 24, 2021 at 04:02:13AM -0500, Xionghu Luo wrote: > vmrghb only accepts permute index {0, 16, 1, 17, 2, 18, 3, 19, 4, 20, > 5, 21, 6, 22, 7, 23} no matter for BE or LE in ISA, similarly for vmrghlb. (vmrglb) > + if (BYTES_BIG_ENDIAN) > + emit_insn ( > + gen_altivec_vmrghb_direct (operands[0], operands[1], operands[2])); > + else > + emit_insn ( > + gen_altivec_vmrglb_direct (operands[0], operands[2], operands[1])); Please don't indent like that, it doesn't match what we do elsewhere. For better or for worse (for worse imo), we use deep hanging indents. If you have to, you can do something like rtx insn; if (BYTES_BIG_ENDIAN) insn = gen_altivec_vmrghb_direct (operands[0], operands[1], operands[2]); else insn = gen_altivec_vmrglb_direct (operands[0], operands[2], operands[1]); emit_insn (insn); (this is better even, in that it has only one emit_insn), or even rtx (*fun) () = BYTES_BIG_ENDIAN ? gen_altivec_vmrghb_direct : gen_altivec_vmrglb_direct; if (!BYTES_BIG_ENDIAN) std::swap (operands[1], operands[2]); emit_insn (fun (operands[0], operands[1], operands[2])); Well, C++ does not allow that last example like that, sigh, so rtx (*fun) (rtx, rtx, rtx) = BYTES_BIG_ENDIAN ? gen_altivec_vmrghb_direct : gen_altivec_vmrglb_direct; This is shorter than the other two options ;-) > +(define_insn "altivec_vmrghb_direct" > [(set (match_operand:V16QI 0 "register_operand" "=v") > + (vec_select:V16QI This should be indented one space more. > "TARGET_ALTIVEC" > "@ > - xxmrghw %x0,%x1,%x2 > - vmrghw %0,%1,%2" > + xxmrghw %x0,%x1,%x2 > + vmrghw %0,%1,%2" The original indent was correct, please restore. > - emit_insn (gen_altivec_vmrghw_direct (operands[0], ve, vo)); > + emit_insn (gen_altivec_vmrghw_direct_v4si (operands[0], ve, vo)); When you see a mode as part of a pattern name, chances are that it will be a good candidate for using parameterized names with. (But don't do that now, just keep it in mind as a nice cleanup to do). > @@ -23022,8 +23022,8 @@ altivec_expand_vec_perm_const (rtx target, rtx op0, rtx op1, > : CODE_FOR_altivec_vmrglh_direct), > { 0, 1, 16, 17, 2, 3, 18, 19, 4, 5, 20, 21, 6, 7, 22, 23 } }, > { OPTION_MASK_ALTIVEC, > - (BYTES_BIG_ENDIAN ? CODE_FOR_altivec_vmrghw_direct > - : CODE_FOR_altivec_vmrglw_direct), > + (BYTES_BIG_ENDIAN ? CODE_FOR_altivec_vmrghw_direct_v4si > + : CODE_FOR_altivec_vmrglw_direct_v4si), The correct way is to align the ? and the : (or put everything on one line of course, if that fits) The parens around this are not needed btw, and are a distraction. > --- a/gcc/testsuite/gcc.target/powerpc/builtins-1.c > +++ b/gcc/testsuite/gcc.target/powerpc/builtins-1.c > @@ -317,10 +317,10 @@ int main () > /* { dg-final { scan-assembler-times "vctuxs" 2 } } */ > > /* { dg-final { scan-assembler-times "vmrghb" 4 { target be } } } */ > -/* { dg-final { scan-assembler-times "vmrghb" 5 { target le } } } */ > +/* { dg-final { scan-assembler-times "vmrghb" 6 { target le } } } */ > /* { dg-final { scan-assembler-times "vmrghh" 8 } } */ > -/* { dg-final { scan-assembler-times "xxmrghw" 8 } } */ > -/* { dg-final { scan-assembler-times "xxmrglw" 8 } } */ > +/* { dg-final { scan-assembler-times "xxmrghw" 4 } } */ > +/* { dg-final { scan-assembler-times "xxmrglw" 4 } } */ > /* { dg-final { scan-assembler-times "vmrglh" 8 } } */ > /* { dg-final { scan-assembler-times "xxlnor" 6 } } */ > /* { dg-final { scan-assembler-times {\mvpkudus\M} 1 } } */ > @@ -347,7 +347,7 @@ int main () > /* { dg-final { scan-assembler-times "vspltb" 6 } } */ > /* { dg-final { scan-assembler-times "vspltw" 0 } } */ > /* { dg-final { scan-assembler-times "vmrgow" 8 } } */ > -/* { dg-final { scan-assembler-times "vmrglb" 5 { target le } } } */ > +/* { dg-final { scan-assembler-times "vmrglb" 4 { target le } } } */ > /* { dg-final { scan-assembler-times "vmrglb" 6 { target be } } } */ > /* { dg-final { scan-assembler-times "vmrgew" 8 } } */ > /* { dg-final { scan-assembler-times "vsplth" 8 } } */ Are those changes correct? It looks like a vmrglb became a vmrghb, and that 4 each of xxmrghw and xxmrglw disappeared? Both seem wrong? Segher