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 031D03858D3C for ; Tue, 21 Feb 2023 11:05:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 031D03858D3C 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 31LB4of7024546; Tue, 21 Feb 2023 05:04:50 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 31LB4nJB024542; Tue, 21 Feb 2023 05:04:49 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 21 Feb 2023 05:04:48 -0600 From: Segher Boessenkool To: Ajit Agarwal Cc: gcc-patches , bergner@linux.ibm.com Subject: Re: [PATCH] rs6000: fmr gets used instead of faster xxlor [PR93571] Message-ID: <20230221110448.GP25951@gate.crashing.org> References: <16fa34b8-ad8a-20f2-b285-3b3f5bf5d5b2@linux.ibm.com> <20230217172319.GL25951@gate.crashing.org> <5f800afe-f162-0d56-3c78-6ee93610e201@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5f800afe-f162-0d56-3c78-6ee93610e201@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-2.9 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 Tue, Feb 21, 2023 at 02:18:25PM +0530, Ajit Agarwal wrote: > This patch replaces fmr instruction 6 cycles with 2 cycles xxlor instruction > for p7 and p8 architecture. > > I have implemented with switch and cases otherwise it is difficult to accommodate > xxlor with p7 and p8 and fmr for other architectures. Please domn't use a switch, it isn't needed. Instead use the "isa" attribute (with p7v here), and put the preferred alternative first. > rs6000: fmr gets used instead of faster xxlor [PR93571] rs6000: Use xxlor instead of fmr where possible > This patch replaces 6 cycles fmr instruction with xxlor > 2 cycles in p8 and p7 architecture. No, it also does it on all later architectures. Do you have any actual timings (i.e. from hardware, not documentation)? > * config/rs6000/rs6000.md (*movdf_hardfloat64): Replace fmr with xxlor instruction. Line too long. And, that is not what the patch does. Changelog should be totally boring just saying what the patch changes. If the patch changes things other than what thechangelog says your reviewer will think something went missin somewhere :-) > - "@ > - stfd%U0%X0 %1,%0 > - lfd%U1%X1 %0,%1 > - xxlor %0,%1,%1 That is not what is currently in trunk, so your patch cannot apply. > + switch (which_alternative) { > + case 0 : return "stfd%U0%X0 %1,%0"; > + case 1 : return "lfd%U1%X1 %0,%1"; Formatting is all incorrect. We dom't need or want a switch at all, but correct would be: switch (which_alternative) { case 0: return "stfd%U0%X0 %1,%0"; case 1: return "lfd%U1%X1 %0,%1"; etc. > + case 2 : if ((TARGET_VSX || TARGET_P8_VECTOR) > + && !TARGET_P9_VECTOR > + && !TARGET_POWER10) > + return "xxlor %0,%1,%1"; > + else > + return "fmr %0,%1"; Ah, so you are excluding p9 and p10 here. Hrm. That should be written TARGET_VSX && !TARGET_P9_VECTOR, none of the rest is needed; but is that a good idea at all? Please use %xN for VSX arguments whenever possible. If this alternative allows only the low numbered vector registers, that is a hint that you probably should write this differently (and %xN is harmless then). > + return "unreachable"; No, never do that. There is "gcc_unreachable ()" if you need it. So, let's first do actual timings, and see if it is better on p9 and p10 as well (or at least not worse). Segher