From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com [IPv6:2a00:1450:4864:20::22e]) by sourceware.org (Postfix) with ESMTPS id 6480A38582AC for ; Mon, 14 Nov 2022 18:56:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6480A38582AC Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=vrull.eu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=vrull.eu Received: by mail-lj1-x22e.google.com with SMTP id t10so14520750ljj.0 for ; Mon, 14 Nov 2022 10:56:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vrull.eu; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=hGgkS1OYTMenFwS+426Jv2OSQYIchCfOoV7okrl3HPE=; b=VoVMMm631JS5IHLl+PgxeKgvG58YH5skxLEWpJqdzGK0xHSlAHAzgwFBoelh7i6T1f qIRd4GrzHoubvIAxA6KDR3qYndxwBh+u6G7BmQCPsQxECPdo3mZTj2y/XdkBRjORyLT+ CZMa6QpVVc5eocEyL1kRQhMznDFq/p62d2p8WEvOa8fMMJPF4oB6YWXZlVgJIyqlrxGN yHKsV/3QRsnhWfLCP/PZFYABNjBskUOH5hL43CBcf4INEb9o0jdmV2bGbpsEOG6Z+WBq vWmiFuff4KR3IT/e8szQlpJhYkf0jJE81klgtlm/Q6WAtgFUkfa2DESI+XvMkFi5j8WI P1nA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=hGgkS1OYTMenFwS+426Jv2OSQYIchCfOoV7okrl3HPE=; b=yxnFTYgMLOX7dRkT2AKI6PMWCu39RPLn0s31QykPf9TNglC+Tb3qDip/fIvOKeAFtE RNYe8F2Sbwsux+K9yWod5ZDSdGXXNXaGmFW4d3NO6VjuGpVIFBXQaZ0rh3mWemaN3ok+ PE3rA/q1sJvYpc/PhrhUqXKHo4EeNVVtZ1F/utwJN6jCEkpcel8ZhT4bsSuxPE040Yv7 /R3TOb1wNckcDU5KANDBGyxZB8J7Z59RtqdIN3sHgojKoS8i7h3BmE8f/AWy1J3WAKcC rdRQ1jHBmqY37y5ZI0PfUbDrq9l1OpGhiSEB94wdMg6GJlq71udJv/h7MW7yYzF3Fdl4 jqpQ== X-Gm-Message-State: ANoB5plePK42qN8tAdOTyN3O1FJQPOiI4K5bK+QrnQxOfm2b25ZuUrA6 zQoDNZGj6UUpQ9sqnmYKj5HmcseNT9gOVvIgk4nPvQ== X-Google-Smtp-Source: AA0mqf4Bli24q8mM6O8KmxmLk8SDT1DJf6G+tCucCe2HqKEO48lpSHYQwhEhJv+eSRgbwgjDwM2gt5bCFY5+BFlbdDA= X-Received: by 2002:a2e:9e55:0:b0:277:e8e:8d90 with SMTP id g21-20020a2e9e55000000b002770e8e8d90mr4843057ljk.243.1668452160937; Mon, 14 Nov 2022 10:56:00 -0800 (PST) MIME-Version: 1.0 References: <20221113204824.4062042-1-philipp.tomsich@vrull.eu> <20221113204824.4062042-3-philipp.tomsich@vrull.eu> In-Reply-To: From: Philipp Tomsich Date: Mon, 14 Nov 2022 19:55:49 +0100 Message-ID: Subject: Re: [PATCH v2 2/2] RISC-V: Add instruction fusion (for ventana-vt1) To: Jeff Law Cc: gcc-patches@gcc.gnu.org, Palmer Dabbelt , Vineet Gupta , Jeff Law , Kito Cheng , Christoph Muellner Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP 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: On Mon, 14 Nov 2022 at 17:06, Jeff Law wrote: > > > On 11/13/22 13:48, Philipp Tomsich wrote: > > The Ventana VT1 core supports quad-issue and instruction fusion. > > This implemented TARGET_SCHED_MACRO_FUSION_P to keep fusible sequences > > together and adds idiom matcheing for the supported fusion cases. > > > > gcc/ChangeLog: > > > > * config/riscv/riscv.cc (enum riscv_fusion_pairs): Add symbolic > > constants to identify supported fusion patterns. > > (struct riscv_tune_param): Add fusible_op field. > > (riscv_macro_fusion_p): Implement. > > (riscv_fusion_enabled_p): Implement. > > (riscv_macro_fusion_pair_p): Implement and recoginze fusible > > idioms for Ventana VT1. > > (TARGET_SCHED_MACRO_FUSION_P): Point to riscv_macro_fusion_p. > > (TARGET_SCHED_MACRO_FUSION_PAIR_P): Point to riscv_macro_fusion_pair_p. > > You know the fusion rules for VT1 better than I... I'm happy to largely > defer to you on this. > > I do wonder if going forward hand matching RTL like this is going to be > an unmaintainable mess and whether or not we would be better served > using insn attributes to describe instruction fusion. I had thought about that, too. In the end our team decided to stay away from it for the time being: fusion frequently needs to look at second-level properties and whether the first instruction's output register is overwritten by the second instruction. So we kept with the same stereotype of idiom-matching that is also used for AArch64 today. That said, both the RISC-V and the AArch64 implementations of this are on my list of things to refactor in a quiet hour. > > > > > > > Signed-off-by: Philipp Tomsich > > --- > > > > Changes in v2: > > - Update fusion patterns and catch some missing idioms/fusion pairs. > > > > gcc/config/riscv/riscv.cc | 219 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 219 insertions(+) > > > > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc > > index 31d651f8744..43ba520885c 100644 > > --- a/gcc/config/riscv/riscv.cc > > +++ b/gcc/config/riscv/riscv.cc > > > > +static bool > > +riscv_macro_fusion_pair_p (rtx_insn *prev, rtx_insn *curr) > > +{ > > + rtx prev_set = single_set (prev); > > + rtx curr_set = single_set (curr); > > + /* prev and curr are simple SET insns i.e. no flag setting or branching. */ > > + bool simple_sets_p = prev_set && curr_set && !any_condjump_p (curr); > > + > > + if (!riscv_macro_fusion_p ()) > > + return false; > > + > > + if (simple_sets_p && (riscv_fusion_enabled_p (RISCV_FUSE_ZEXTW) || > > + riscv_fusion_enabled_p (RISCV_FUSE_ZEXTH))) > > Formatting nit. Bring the && down to a new line and if you still need a > line break for the "||", then the "||" should be on a new line as > well. Something like this... > > > if (simple_sets_p > && (riscv_fusion_enabled_p (RISCV_FUSE_ZEXTW > > || riscv_fusion_enabled_p (RISCV_FUSE_ZEXTH))) > > > > + && REGNO (XEXP (SET_SRC (curr_set), 0)) == REGNO(SET_DEST (curr_set)) > > Space before open paren on this line. > > > > > > + && (( INTVAL (XEXP (SET_SRC (curr_set), 1)) == 32 > > + && riscv_fusion_enabled_p(RISCV_FUSE_ZEXTW) ) > > + || ( INTVAL (XEXP (SET_SRC (curr_set), 1)) < 32 > > + && riscv_fusion_enabled_p(RISCV_FUSE_ZEXTWS)))) > > Extraneous spaces after the open parens before INTVALs above. > > > > + && REGNO (XEXP (SET_SRC (curr_set), 0)) == REGNO(SET_DEST (curr_set)) > > Missing whitespace before open paren on this line. > > > OK with the nits fixed. Applied to master with these fixes (and a fix for the typo in the commit message that Jakub spotted). Thanks! Philipp.