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 0629F3858D28 for ; Tue, 20 Jun 2023 20:06:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0629F3858D28 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 35KK5F1j017527; Tue, 20 Jun 2023 15:05:16 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 35KK5FxX017526; Tue, 20 Jun 2023 15:05:15 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 20 Jun 2023 15:05:15 -0500 From: Segher Boessenkool To: Michael Meissner , gcc-patches@gcc.gnu.org, "Kewen.Lin" , David Edelsohn , Peter Bergner Subject: Re: [PATCH, V6] Fix power10 fusion and -fstack-protector, PR target/105325 Message-ID: <20230620200515.GG19790@gate.crashing.org> References: 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=-2.6 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,KAM_NUMSUBJECT,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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! The patch looks great now, thanks you! But the commit message needs some work: First off, the subject, which is a short (50 character max!) summary of what the patch is about. Fix power10 fusion and -fstack-protector, PR target/105325 There is absolutely nothing to do with stack protector, it does not belong in the commit message at all, and certainly not in the subject! On Tue, Jun 13, 2023 at 10:14:02PM -0400, Michael Meissner wrote: > This patch fixes an issue where if you use the -fstack-protector and > -mcpu=power10 options and you have a large stack frame, the GCC compiler will > generate a LWA instruction with a large offset. That is not the core issue, it is just one example where things went wrong. That prompted this patch, sure, so you can talk about that ten or so lines down if you think it is important (I don't fwiw), but not at the start here. You should just say what was wrong, so people with a short attention span can just skip this patch when looking through git log (and even earlier if the subject would be good). Commit messages are for *future* users. Not for getting your patch approved. > Here is the initial fused initial insn that was created. It refers to the > stack location based off of the virtrual frame pointer: The soft frame pointer, not a virtual one. For PowerPC this is not a real register and LRA will eventually replace it, sure. "Virtual" here in GCC has a very specific meaning; virtual things are replaced very soon after expand. > When the split2 pass is run after reload has finished the ds_form_mem_operand > predicate that was used for lwa and ld no longer returns true. Yes. It is the wrong predicate to use here. *That* is the problem. > 2) Delete ds_form_mem_operand since it is no longer used. ... and we don't expect to use it any time soon. > 3) Use the "YZ" constraints for ld/lwa instead of "m". Yes, constraints and predicates. > * config/rs6000/genfusion.pl (gen_ld_cmpi_p10_one): Fix problems that > allowed prefixed lwa to be generated. You should not say what the *old* code did, in the changelog! > --- a/gcc/config/rs6000/genfusion.pl > +++ b/gcc/config/rs6000/genfusion.pl > @@ -61,20 +61,30 @@ sub gen_ld_cmpi_p10_one > my $mempred = "non_update_memory_operand"; > my $extend; > > + # We need to special case lwa. The prefixed_load_p function in rs6000.cc > + # (which determines if a load instruction is prefixed) uses the fact that the > + # register mode is different from the memory mode, and that the sign_extend > + # attribute is set to use DS-form rules for the address instead of D-form. > + # If the register size is the same, prefixed_load_p assumes we are doing a > + # lwz. We change to use an lwz and word compare if we don't need to sign > + # extend the SImode value. Otherwise if we need the value, we need to > + # make sure the insn is marked as ds-form. > + my $lwa_insn = ($lmode eq "SI" && $ccmode eq "CC"); That is a pretty bad name, the variable does not hold an "insn" in any way, shape, or form. It is hardish to give it a good name because it mixes two questions into one variable? You can just repeat the tiny conditions wherever you use them, and the code would be more readable (and less cryptic!) > + if ($lwa_insn && $cmp_size eq "d") { Name it "cmp_size_char" maybe? "cmp_size" suggests a number. > --- /dev/null > +++ b/gcc/testsuite/g++.target/powerpc/pr105325.C > @@ -0,0 +1,26 @@ > +/* { dg-do assemble } */ > +/* { dg-require-effective-target lp64 } */ > +/* { dg-require-effective-target power10_ok } */ > +/* { dg-require-effective-target powerpc_prefixed_addr } */ > +/* { dg-options "-O2 -mdejagnu-cpu=power10 -fstack-protector" } */ > + > +/* Test that power10 fusion does not generate an LWA/CMPDI instruction pair > + instead of PLWZ/CMPWI. Ultimately the code was dying because the fusion > + load + compare -1/0/1 patterns did not handle the possibility that the load > + might be prefixed. The -fstack-protector option is needed to show the > + bug. */ Mention the PR number somewhere in the text as well? For grep etc. Okay for trunk, with some more reasonable commmit message. Thank you! Also okay for all backports. Segher