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 76A77385480B; Thu, 13 May 2021 10:50:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 76A77385480B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=segher@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 14DAnWJD019913; Thu, 13 May 2021 05:49:32 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 14DAnVnp019909; Thu, 13 May 2021 05:49:31 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 13 May 2021 05:49:31 -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: Fix wrong code generation for vec_sel [PR94613] Message-ID: <20210513104931.GG10366@gate.crashing.org> References: <20210430063258.2774866-1-luoxhu@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210430063258.2774866-1-luoxhu@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=ham 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: Thu, 13 May 2021 10:50:36 -0000 Hi! On Fri, Apr 30, 2021 at 01:32:58AM -0500, Xionghu Luo wrote: > The vsel instruction is a bit-wise select instruction. Using an > IF_THEN_ELSE to express it in RTL is wrong and leads to wrong code > being generated in the combine pass. Per element selection is a > subset of per bit-wise selection,with the patch the pattern is > written using bit operations. But there are 8 different patterns > to define "op0 := (op1 & ~op3) | (op2 & op3)": > > (~op3&op1) | (op3&op2), > (~op3&op1) | (op2&op3), > (op3&op2) | (~op3&op1), > (op2&op3) | (~op3&op1), > (op1&~op3) | (op3&op2), > (op1&~op3) | (op2&op3), > (op3&op2) | (op1&~op3), > (op2&op3) | (op1&~op3), > > Combine pass will swap (op1&~op3) to (~op3&op1) due to commutative > canonical, which could reduce it to the FIRST 4 patterns, but it won't > swap (op2&op3) | (~op3&op1) to (~op3&op1) | (op2&op3), so this patch > handles it with two patterns with different NOT op3 position and check > equality inside it. Yup, that latter case does not have canonicalisation rules. Btw, not only combine does this canonicalisation: everything should, non-canonical RTL is invalid RTL (in the instruction stream, you can do everything in temporary code of course, as long as the RTL isn't malformed). > -(define_insn "*altivec_vsel" > +(define_insn "altivec_vsel" > [(set (match_operand:VM 0 "altivec_register_operand" "=v") > - (if_then_else:VM > - (ne:CC (match_operand:VM 1 "altivec_register_operand" "v") > - (match_operand:VM 4 "zero_constant" "")) > - (match_operand:VM 2 "altivec_register_operand" "v") > - (match_operand:VM 3 "altivec_register_operand" "v")))] > - "VECTOR_MEM_ALTIVEC_P (mode)" > - "vsel %0,%3,%2,%1" > + (ior:VM > + (and:VM > + (not:VM (match_operand:VM 3 "altivec_register_operand" "v")) > + (match_operand:VM 1 "altivec_register_operand" "v")) > + (and:VM > + (match_operand:VM 2 "altivec_register_operand" "v") > + (match_operand:VM 4 "altivec_register_operand" "v"))))] > + "VECTOR_UNIT_ALTIVEC_OR_VSX_P (mode) > + && (rtx_equal_p (operands[2], operands[3]) > + || rtx_equal_p (operands[4], operands[3]))" > + { > + if (rtx_equal_p (operands[2], operands[3])) > + return "vsel %0,%1,%4,%3"; > + else > + return "vsel %0,%1,%2,%3"; > + } > [(set_attr "type" "vecmove")]) That rtx_equal_p stuff is nice and tricky, but it is a bit too tricky I think. So please write this as two patterns (and keep the expand if that helps). > +(define_insn "altivec_vsel2" (same here of course). > ;; Fused multiply add. > diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c > index f5676255387..d65bdc01055 100644 > --- a/gcc/config/rs6000/rs6000-call.c > +++ b/gcc/config/rs6000/rs6000-call.c > @@ -3362,11 +3362,11 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { > RS6000_BTI_V2DI, RS6000_BTI_V2DI, RS6000_BTI_V2DI, RS6000_BTI_unsigned_V2DI }, > { ALTIVEC_BUILTIN_VEC_SEL, ALTIVEC_BUILTIN_VSEL_2DI, > RS6000_BTI_V2DI, RS6000_BTI_V2DI, RS6000_BTI_V2DI, RS6000_BTI_V2DI }, > - { ALTIVEC_BUILTIN_VEC_SEL, ALTIVEC_BUILTIN_VSEL_2DI, > + { ALTIVEC_BUILTIN_VEC_SEL, ALTIVEC_BUILTIN_VSEL_2DI_UNS, Are the _uns things still used for anything? But, let's not change this until Bill's stuff is in :-) Why do you want to change this here, btw? I don't understand. > + if (target == 0 > + || GET_MODE (target) != tmode > + || ! (*insn_data[icode].operand[0].predicate) (target, tmode)) No space after ! and other unary operators (except for casts and other operators you write with alphanumerics, like "sizeof"). I know you copied this code, but :-) > @@ -15608,8 +15606,6 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx op_true, rtx op_false, > case GEU: > case LTU: > case LEU: > - /* Mark unsigned tests with CCUNSmode. */ > - cc_mode = CCUNSmode; > > /* Invert condition to avoid compound test if necessary. */ > if (rcode == GEU || rcode == LEU) So this is related to the _uns thing. Could you split off that change? Probably as an earlier patch (but either works for me). > @@ -15629,6 +15625,9 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx op_true, rtx op_false, > if (!mask) > return 0; > > + if (mask_mode != dest_mode) > + mask = simplify_gen_subreg (dest_mode, mask, mask_mode, 0); Indent just two characters please: line continuations (usually) align, but indents do not. Can you fold vsel and xxsel together completely? They have exactly the same semantics! This does not have to be in this patch of course. Thanks, Segher