From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x536.google.com (mail-ed1-x536.google.com [IPv6:2a00:1450:4864:20::536]) by sourceware.org (Postfix) with ESMTPS id 5EBAA3858C3A for ; Tue, 23 May 2023 08:06:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5EBAA3858C3A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-ed1-x536.google.com with SMTP id 4fb4d7f45d1cf-510b4e488e4so1028728a12.3 for ; Tue, 23 May 2023 01:06:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684829204; x=1687421204; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:cc:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=WBRJ9SAstktTotJ+ClWL7JAg0lB+RrWvXJJb1/6uRs4=; b=eYTrf8ikRGLQ7E9EGWu4uUEs26Tjxl/EjDNxhADgPZYSopMjC49zAA6eM3Mhvh6Xfz +9e5vvGYP7sKKeclt0iJBRG0REB4VOC6pd++Ctlm2aYBlJFxOY2za4VZR/FxH14kCGLP C4CAMqfgBrpDyM6NAkRPw5rqwjXrcknANQV7VRIbD0M+VeIO3AeXqS4DDk/D7lV4LItw 0ZoHyJh9gyOFc9ipBr5XL8sjzrbOAJQeULcQCJzUc1mROvbiPZes2F+CctUaJjY8u5xg Pj0npfZFmvIodVfwBsAtWmpLO6P9KyfR9WXpy6EmDfwD6vN+5f2CKuagm3B9Jg8oGTAI ySXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684829204; x=1687421204; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:cc:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WBRJ9SAstktTotJ+ClWL7JAg0lB+RrWvXJJb1/6uRs4=; b=FR4TUuXXAb9NK9kPHqRIDvpnk3o3arYBkCCZPbD3Q9f7dLA8CjxwhcizIU2++P3lvM beJrqKwaXiW/g4nCRM4oIeWHYdn59hiWiXW4AzQNHeyBWfqnuLK1rPOdy8JbDXh+SMtk 4PcHtd/zaDnbfDNj1lLWw+AXnLtYP2C4UDOmgKOjWyW1K8qhL/AXd/pHSdxAtUMBV8YB yod3fciNKNtFhcc9Ctr2P1jD0Z+tYdOVBv6iepAk3yTpkFPtpqWze3xst0j2yo5o7WtP W6FRZcMnnXh74UGEkdSOm9kNnUv1zC/gZjh+Br+Lx2i1qSlf3J2RrzXq+0Bs3UhsGJJc icPA== X-Gm-Message-State: AC+VfDwY92RB7QUJN5k31zxUI1qvOORETTv7CtpnuaiKaJ98lRcZHEoU H1t6IPXpvPK9L7NetWJu6V0= X-Google-Smtp-Source: ACHHUZ7QKRErZvLN4VuBvagirDx/jRluJl263QoD91BYW78tyG7S72mGtF2uW5gtJA+AjJQKoAkmtQ== X-Received: by 2002:a17:906:dc91:b0:96f:45cd:6c24 with SMTP id cs17-20020a170906dc9100b0096f45cd6c24mr12627735ejc.23.1684829203961; Tue, 23 May 2023 01:06:43 -0700 (PDT) Received: from [192.168.1.23] (ip-046-005-130-086.um12.pools.vodafone-ip.de. [46.5.130.86]) by smtp.gmail.com with ESMTPSA id u10-20020a17090657ca00b00960005e09a3sm4119068ejr.61.2023.05.23.01.06.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 23 May 2023 01:06:43 -0700 (PDT) Message-ID: <3cf370ef-adab-2f36-4361-5474de68386f@gmail.com> Date: Tue, 23 May 2023 10:06:42 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Cc: rdapp.gcc@gmail.com, kito.cheng@gmail.com, kito.cheng@sifive.com, palmer@dabbelt.com, palmer@rivosinc.com, jeffreyalaw@gmail.com Subject: Re: [PATCH] RISC-V: Refactor the framework of RVV auto-vectorization To: juzhe.zhong@rivai.ai, gcc-patches@gcc.gnu.org References: <20230523060804.61556-1-juzhe.zhong@rivai.ai> Content-Language: en-US From: Robin Dapp In-Reply-To: <20230523060804.61556-1-juzhe.zhong@rivai.ai> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham 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 Juzhe, in general I find the revised structure quite logical and it is definitely an improvement. Some abstraction are still a bit leaky but we can always refactor "on the fly". Some comments on the general parts, skipping over the later details. > bool m_has_dest_p; Why does a store not have a destination (as commented below)? > /* It't true if the pattern uses all trues mask operand. */ > bool m_use_all_trues_mask_p; m_all_unmasked_p or m_fully_unmasked_p? > /* It's true if the pattern uses undefined merge operand. */ > bool m_use_undef_merge_p; Apart from the insn-centric name, couldn't we also decide this based on the context later? In the vector-builtins.cc we have use_real_mask_p and use_real_merge_p that do this. > bool m_has_avl_p; This means "has avl operand" I suppose? From the caller's point of view (and also the vsetvl pass) something like "needs avl" or so would be more descriptive but undecided here. > bool m_vlmax_p; > bool m_has_tail_policy_p; > bool m_has_mask_policy_p; Do we need to expose these in the constructor? As far as I can tell we can decide later whether the instruction has a policy or not (as I did in my patch, depending on whether all inputs are masks or so). > enum tail_policy m_tail_policy; > enum mask_policy m_mask_policy; > machine_mode m_dest_mode; > machine_mode m_mask_mode; Having the mask mode be automatically deduced from the destination is good, it was just obnoxious before to pass ... > Currently, we have "emit_vlmax_tany_many" and "emit_nonvlmax_tany_many". I don't particularly like the names ;) Going back to vlmax and nonvlmax I don't mind but do we really need to have the policies encoded in the name now? Especially since "many" is a word and the default is ANY anyway. Why not emit_vlmax_insn/emit_vlmax_op for now and add the tu/mu later? > #define RVV_BINOP_NUM 3 (number including the output) Could make this into an "instruction type" rather than just a number (i.e. RVV_BINOP) and then set the number of operands internally according to the type. This would also make it clearer in case we later want to set other options depending on the type. > Then just use emit_vlmax_tany_many (...RVV_BINOP_NUM...) > > So, if we support ternary operation in the future. It's quite simple: > #define RVV_TERNOP_NUM 4 (number including the output) > emit_vlmax_tany_many (...RVV_BINOP_NUM...) > > "*_tany_many" means we are using tail any and mask any. > > We will definitely need tail undisturbed or mask undisturbed when we support these patterns > in middle-end. It's very simple to extend such helper base on current framework: > > we can do that in the future like this: > > void > emit_nonvlmax_tu_mu (unsigned icode, int op_num, rtx *ops) > { > machine_mode data_mode = GET_MODE (ops[0]); > machine_mode mask_mode = get_mask_mode (data_mode).require (); > /* The number = 11 is because we have maximum 11 operands for > RVV instruction patterns according to vector.md. */ You can just drop the "The number = 11 is because" and say "We have a maximum of 11 operands for...". > insn_expander<11> e (/*OP_NUM*/ op_num, /*HAS_DEST_P*/ true, > /*USE_ALL_TRUES_MASK_P*/ true, > /*USE_UNDEF_MERGE_P*/ true, /*HAS_AVL_P*/ true, > /*VLMAX_P*/ false, > /*HAS_TAIL_POLICY_P*/ true, /*HAS_MASK_POLICY_P*/ true, > /*TAIL_POLICY*/ TAIL_UNDISTURBED, /*MASK_POLICY*/ MASK_UNDISTURBED, > /*DEST_MODE*/ data_mode, /*MASK_MODE*/ mask_mode); > e.emit_insn ((enum insn_code) icode, ops); > } The eleven arguments seem a bit clunky here ;) I would suggest changing this again in the future bur for now let's just go ahead with it in order to make progress. > - riscv_vector::emit_len_op (code_for_pred_mov (mode), operands[0], > - operands[1], operands[2], mode); > + riscv_vector::emit_nonvlmax_tany_many (code_for_pred_mov (mode), > + RVV_UNOP_NUM, operands); > DONE; > }) The rtx operands[] array I like least of the changes in this patch. It's essentially an untyped array whose meaning is dependent on context containing source operands and the length that is sometimes empty and sometimes not. I can't think of something that wouldn't complicate things though but before we at least had functions called _len that would take a length (NULL or not) and _vlmax that wouldn't. It's pretty easy to mess up here on the caller's side. That said, again, we can always refactor again and let's not let perfect be the enemy of good here. All in all it is not more complicated now than it was before so we should go ahead IMHO. Regards Robin