From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpbgau1.qq.com (smtpbgau1.qq.com [54.206.16.166]) by sourceware.org (Postfix) with ESMTPS id ACBA03858D37 for ; Thu, 31 Aug 2023 10:00:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org ACBA03858D37 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivai.ai Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivai.ai X-QQ-mid: bizesmtp71t1693475994t0iqze7p Received: from [10.101.11.9] ( [113.104.210.73]) by bizesmtp.qq.com (ESMTP) with id ; Thu, 31 Aug 2023 17:59:53 +0800 (CST) X-QQ-SSF: 01400000000000C0F000000A0000000 X-QQ-FEAT: Zre9CpKCW9OYaNzhfDny4jZhY63FlLTKKnqNT5kX/wPSizHjrBHFooyyN8w4V MLxltXLlWcTtJVt23L5FoYGo9W3UdIkDColwwS8Z/+alK800A4kzFybJiU2eWShJO1P8N80 G2WGoG6sjk1G4ACCslZumuzlCjBW6B6xcnQ9e4PQJ1OsOFGsQXtjUuOynP9fHi1hMfJSMEP JUVlBvBnnkFZ7FdVWEb/Pr9L+3yLsLI+wKf7cnSFkR3DDdhgSF1fqPk1IFF9PfoBhgbm2ez e4vwmBDcEzztccswh5KdtJNQwd3OV5n5mtSntPmsw4hJ7QlaTtSxZIb0RHsoGo5H883L2VF EpoNiaEgVKG4dHgDfZyMegBKNt4ufaVpvX8LfrRqILOPmUhvwKA3qM7dQ6RA4dPjZT6XJBi X-QQ-GoodBg: 2 X-BIZMAIL-ID: 15029813578621365124 Message-ID: <8A3CB7462E9A673F+37460eeb-e972-4129-8557-1ee9dd030e95@rivai.ai> Date: Thu, 31 Aug 2023 17:59:53 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] RISC-V: Refactor and clean emit_{vlmax,nonvlmax}_xxx functions Content-Language: en-US To: Robin Dapp , gcc-patches@gcc.gnu.org Cc: juzhe.zhong@rivai.ai, kito.cheng@gmail.com, palmer@rivosinc.com, jeffreyalaw@gmail.com References: <20230830115327.1296036-1-lehua.ding@rivai.ai> From: Lehua Ding In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:rivai.ai:qybglogicsvrgz:qybglogicsvrgz6a-0 X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,FORGED_MUA_MOZILLA,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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 Robin, Thanks for these comments. On 2023/8/31 17:16, Robin Dapp wrote: > Hi Lehua, > > thanks, this definitely goes into the direction of what I had in mind and > simplifies a lot of the reduntant emit_... so it's good to have it. > > I was too slow for a detailed response :) So just some high-level comments. > > One thing I noticed is the overloading of "MASK_OP", we use it as > "operation on masks" i.e. an insn as well as "mask policy". IMHO we could > get rid of UNARY_MASK_OP and BINARY_MASK_OP and just decide whether to > add a mask policy depending on if all operands are masks (the same way we > did before). I think I should change the marco name here form __MASK_OP to __NORMAL_OP_MASK. MASK_OP means this is a insn operate on mask operands. I lift up it here because I want to simplify the emit_insn method. > Related, and seeing that the MASK in UNARY_MASK_OP is somewhat redundant, > I feel we still mix concerns a bit. For example it is not obvious, from > the name at least, why a WIDEN_TERNARY_OP does not have a merge operand > and the decision making seems very "enum centered" now :D As the above says, UNARY_MASK_OP means it is an operator of mask operands and itself don't have mask operand. If the operation depends on a mask, then the name should be UNARY_MASK_OP_TAMA. For the WIDEN_TERNARY_OP, This is because of the design problem of vwmacc pattern (as bellow), maybe it can be unified with wmacc and then WIDEN_TERNARY_OP is not needed. (define_insn "@pred_widen_mul_plus" [(set (match_operand:VWEXTI 0 "register_operand") (if_then_else:VWEXTI (unspec: [(match_operand: 1 "vector_mask_operand") (match_operand 5 "vector_length_operand") (match_operand 6 "const_int_operand") (match_operand 7 "const_int_operand") (match_operand 8 "const_int_operand") (reg:SI VL_REGNUM) (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE) (plus:VWEXTI (mult:VWEXTI (any_extend:VWEXTI (match_operand: 3 "register_operand")) (any_extend:VWEXTI (match_operand: 4 "register_operand"))) (match_operand:VWEXTI 2 "register_operand")) (match_dup 2)))] "TARGET_VECTOR" "vwmacc.vv\t%0,%3,%4%p1" [(set_attr "type" "viwmuladd") (set_attr "mode" "")]) > In general we use the NULLARY, UNARY, BINARY, TERNARY prefixes just > to determine the number of sources which doesn't seem really necessary > because a user of e.g. NEG will already know that there only is one > source - he already specified it and currently needs to, redundantly, > say UNARY again. From the caller's point of view, he does know how many sources, but the emit_insn method needs to use this flag(NULLARY_OP_P, UNARY_OP_P) to determine how many sources to add. Oh, you mean we can get oerands number from insn_data[icode].n_operands? In this case, we don't really need to distinguish between NULLARY, UNARY and ect. > If we split off the destination and sources from mask, merge and the rest > we could ditch them altogether. > > What about > emit_(non)vlmax_insn (icode, *operands (just dest and sources), > mask, merge, tail/mask policy, frm) > > with mask defaulting to NULL and merge defaulting to VUNDEF? So ideally, > and in the easy case, the call would just degenerate to > emit_..._insn (icode, operands). > > I realize this will cause some complications on the "other side" but with > the enum in place it should still be doable? That is to say, when the user needs to emit a COND_NEG, do you need to call it like this?: emit_vlmax_insn (pred_neg, ops, mask, vundef(), ta, ma) Is there too much focus on operands that don't need to be passed in? Like merge, tail and mask policy operands. Maybe I don't understand enough here, can you explain it in some more detail? -- Best, Lehua