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 CC6663858421 for ; Tue, 20 Dec 2022 13:20:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CC6663858421 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass 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 2BKDJ5YA012289; Tue, 20 Dec 2022 07:19:05 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 2BKDJ41f012288; Tue, 20 Dec 2022 07:19:04 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 20 Dec 2022 07:19:04 -0600 From: Segher Boessenkool To: "Kewen.Lin" Cc: GCC Patches , Peter Bergner , Michael Meissner , David Edelsohn Subject: Re: [PATCH] rs6000: Fix some issues related to Power10 fusion [PR104024] Message-ID: <20221220131904.GR25951@gate.crashing.org> References: <009fda27-7119-6de8-8dbe-51126bdfca12@linux.ibm.com> <20221214222944.GR25951@gate.crashing.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,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! On Mon, Dec 19, 2022 at 02:13:49PM +0800, Kewen.Lin wrote: > on 2022/12/15 06:29, Segher Boessenkool wrote: > > On Wed, Nov 30, 2022 at 04:30:13PM +0800, Kewen.Lin wrote: > >> --- a/gcc/config/rs6000/genfusion.pl > >> +++ b/gcc/config/rs6000/genfusion.pl > >> @@ -167,7 +167,7 @@ sub gen_logical_addsubf > >> $inner_comp, $inner_inv, $inner_rtl, $inner_op, $both_commute, $c4, > >> $bc, $inner_arg0, $inner_arg1, $inner_exp, $outer_arg2, $outer_exp, > >> $ftype, $insn, $is_subf, $is_rsubf, $outer_32, $outer_42,$outer_name, > >> - $fuse_type); > >> + $fuse_type, $constraint_cond); > >> KIND: foreach $kind ('scalar','vector') { > >> @outer_ops = @logicals; > >> if ( $kind eq 'vector' ) { > >> @@ -176,12 +176,14 @@ sub gen_logical_addsubf > >> $pred = "altivec_register_operand"; > >> $constraint = "v"; > >> $fuse_type = "fused_vector"; > >> + $constraint_cond = "VECTOR_UNIT_ALTIVEC_OR_VSX_P (mode) && "; > >> } else { > >> $vchr = ""; > >> $mode = "GPR"; > >> $pred = "gpc_reg_operand"; > >> $constraint = "r"; > >> $fuse_type = "fused_arith_logical"; > >> + $constraint_cond = ""; > >> push (@outer_ops, @addsub); > >> push (@outer_ops, ( "rsubf" )); > >> } > > > > I don't like this at all. Please use the "isa" attribute where needed? > > Or do you need more in some cases? But, again, separate patch. > > This is to add one more condition for those define_insns, for example: Sure, I understand that. What I don't like is the generator program is much too big and unstructured already, and this doesn't help at all; it makes it quite a bit worse even. > It's to avoid the pseudo whose mode isn't available for register constraint v > causes ICE during reload. I'm not sure how the "isa" attribute helps here, > could you elaborate it? Yeah, it doesn't help. The condition implied by the isa attribute is not added to the insn condition automatically; doing that could be too expensive, and disruptive as well. Something for stage 1 :-) > >> + if (TARGET_POWER10 > >> + && (rs6000_isa_flags_explicit & OPTION_MASK_P10_FUSION) == 0) > >> + rs6000_isa_flags |= OPTION_MASK_P10_FUSION; > >> + else if (!TARGET_POWER10 && TARGET_P10_FUSION) > >> + rs6000_isa_flags &= ~OPTION_MASK_P10_FUSION; > > > > That's not right. If you want something like this you should check for > > TARGET_POWER10 whenever you check for TARGET_P10_FUSION; but there > > really is no reason at all to disable P10 fusion on other CPUs (neither > > newer nor older!). > > Good point, and I just noticed that we should check tune setting instead > of TARGET_POWER10 here? Something like: > > if (!(rs6000_isa_flags_explicit & OPTION_MASK_P10_FUSION)) > { > if (processor_target_table[tune_index].processor == PROCESSOR_POWER10) > rs6000_isa_flags |= OPTION_MASK_P10_FUSION; > else > rs6000_isa_flags &= ~OPTION_MASK_P10_FUSION; > } Yeah that looks better :-) Maybe you can restructure the Perl code a bit in a first patch, and then add the insn condition? If you're not comfortable with Perl, I'll deal with it, just update the patch. Thanks, Segher