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 C51E03858D32 for ; Mon, 27 Feb 2023 16:29:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C51E03858D32 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 31RGS7tc022086; Mon, 27 Feb 2023 10:28:07 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 31RGS6ru022085; Mon, 27 Feb 2023 10:28:06 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 27 Feb 2023 10:28:06 -0600 From: Segher Boessenkool To: Surya Kumari Jangala Cc: GCC Patches , Peter Bergner , meissner@linux.ibm.com Subject: Re: [PATCH] swap: Fix incorrect lane extraction by vec_extract() [PR106770] Message-ID: <20230227162806.GY25951@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.9 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: Hi! On Wed, Jan 04, 2023 at 01:58:19PM +0530, Surya Kumari Jangala wrote: > In the routine rs6000_analyze_swaps(), special handling of swappable > instructions is done even if the webs that contain the swappable > instructions are not optimized, i.e., the webs do not contain any > permuting load/store instructions along with the associated register > swap instructions. Doing special handling in such webs will result in > the extracted lane being adjusted unnecessarily for vec_extract. > > Modifying swappable instructions is also incorrect in webs where > loads/stores on quad word aligned addresses are changed to lvx/stvx. > Similarly, in webs where swap(load(vector constant)) instructions are > replaced with load(swapped vector constant), the swappable > instructions should not be modified. > > 2023-01-04 Surya Kumari Jangala > > gcc/ > PR rtl-optimization/106770 > * rs6000-p8swap.cc (rs6000_analyze_swaps): . Please add an entry? Or multiple ones, actually. Describe all changes. > --- a/gcc/config/rs6000/rs6000-p8swap.cc > +++ b/gcc/config/rs6000/rs6000-p8swap.cc > @@ -179,6 +179,9 @@ class swap_web_entry : public web_entry_base > unsigned int special_handling : 4; > /* Set if the web represented by this entry cannot be optimized. */ > unsigned int web_not_optimizable : 1; > + /* Set if the web represented by this entry has been optimized, ie, s/ie/i.e./ > + register swaps of permuting loads/stores have been removed. */ If it really means only exactly this, then the name isn't so good. > + unsigned int web_is_optimized : 1; And if it is as general as the name suggests, then the comment is no good. Which is it? :-) > /* For each load and store in an optimizable web (which implies > the loads and stores are permuting), find the associated > register swaps and mark them for removal. Due to various > - optimizations we may mark the same swap more than once. Also > - perform special handling for swappable insns that require it. */ > + optimizations we may mark the same swap more than once. Fix up > + the non-permuting loads and stores by converting them into > + permuting ones. */ Two spaces after a full stop is correct. Please put that back. Is it a good idea convert from/to swapping load/stores in this pass at all? Doesdn't that belong elsewhere? Like, in combine, where we already should do this. Why does that not work? > - if (!root_entry->web_not_optimizable) > + if (!root_entry->web_not_optimizable) { Blocks start on a new line, indented. > mark_swaps_for_removal (insn_entry, i); > + root_entry->web_is_optimized = true; Indent using tabs where possible. > + swap_web_entry* root_entry > + = (swap_web_entry*)((&insn_entry[i])->unionfind_root ()); Space before *, in all cases. Space before the second (. There are too many brackets here, too. > + /* Perform special handling for swappable insns that require it. No trailing spaces. > + Note that special handling should be done only for those > + swappable insns that are present in webs optimized above. */ > + for (i = 0; i < e; ++i) > + if (insn_entry[i].is_swappable && insn_entry[i].special_handling && > + !(insn_entry[i].special_handling == SH_NOSWAP_LD || > + insn_entry[i].special_handling == SH_NOSWAP_ST)) > { > swap_web_entry* root_entry > = (swap_web_entry*)((&insn_entry[i])->unionfind_root ()); > - if (!root_entry->web_not_optimizable) > + if (root_entry->web_is_optimized) > handle_special_swappables (insn_entry, i); > } Why this change? > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr106770.c > @@ -0,0 +1,20 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target powerpc_p8vector_ok } */ > +/* { dg-options "-mdejagnu-cpu=power8 -O3 " } */ Is -O3 required? Use -O2 if you can. And no trailing spaces please. > +/* { dg-final { scan-assembler-times "xxpermdi" 2 } } */ Those two remaining are superfluous, so comment that please. Segher