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 134263850602 for ; Wed, 14 Dec 2022 22:30:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 134263850602 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 2BEMTkYv022131; Wed, 14 Dec 2022 16:29:46 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 2BEMTjRP022128; Wed, 14 Dec 2022 16:29:45 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Wed, 14 Dec 2022 16:29:44 -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: <20221214222944.GR25951@gate.crashing.org> References: <009fda27-7119-6de8-8dbe-51126bdfca12@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <009fda27-7119-6de8-8dbe-51126bdfca12@linux.ibm.com> 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,KAM_SHORT,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: On Wed, Nov 30, 2022 at 04:30:13PM +0800, Kewen.Lin wrote: > As PR104024 shows, the option -mpower10-fusion isn't guarded by > -mcpu=power10, it causes compiler to fuse for some patterns > even without power10 support and then causes ICE unexpectedly, > this patch is to simply unmask it without power10 support, not > emit any warnings as this option is undocumented. Yes, it mostly exists for debugging purposes (and also for testcase). > Besides, for some define_insns in fusion.md which use constraint > v, it requires the condition VECTOR_UNIT_ALTIVEC_OR_VSX_P > (mode), otherwise it can cause ICE in reload, see test > case pr104024-2.c. Please don't two separate things in one patch. It makes bisecting harder than necessary, and perhaps more interesting to you: it makes writing good changelog entries and commit messages harder. > --- 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. > + 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!). > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr104024-1.c > @@ -0,0 +1,16 @@ > +/* { dg-require-effective-target int128 } */ > +/* { dg-options "-O1 -mdejagnu-cpu=power6 -mpower10-fusion" } */ Does this need -O1? If not, use -O2 please; if so, document it. Segher