Re: [PATCH v2] swap: Fix incorrect lane extraction by vec_extract() [PR106770]

[Surya Kumari Jangala <jskumari@linux.vnet.ibm.com>]

Hi Segher,
My replies are inlined:

On 29/10/23 10:16 am, Segher Boessenkool wrote:
> Hi!
> 
> Please say "rs6000/p8swap:" in the subject, not "swap:" :-)
> 
> On Sun, Sep 10, 2023 at 10:58:32PM +0530, Surya Kumari Jangala wrote:
>> Another issue with always handling swappable instructions is that it is
>> incorrect to do so in webs where loads/stores on quad word aligned
>> addresses are changed to lvx/stvx.
> 
> Why?  Please say why in the commit message (the message you send with
> your patch should be the exact eventual commit message!)

ok, I will add more explanation.

> 
>> gcc/
>> 	PR rtl-optimization/PR106770
>> 	* config/rs6000/rs6000-p8swap.cc (non_permuting_mem_insn): New
>> 	function.
> 
> Please don't break commit message / changelog lines early unnecessarily.
> Lines are 80 chars, the leading tab counts as 8.

ok.

> 
>> +  /* Set if the swappable insns in the web represented by this entry
>> +     have to be fixed. Swappable insns have to be fixed in :
> 
> (no space before colon)

ok.

> 
>> +static bool
>> +non_permuting_mem_insn (swap_web_entry *insn_entry, unsigned int i)
>> +{
>> +  return (insn_entry[i].special_handling == SH_NOSWAP_LD ||
>> +	  insn_entry[i].special_handling == SH_NOSWAP_ST);
>> +}
> 
> "return" is not a function, you don't need parens here.

ok.

> 
>> +/* Convert a non-permuting load/store insn to a permuting one.  */
>> +static void
>> +handle_non_permuting_mem_insn (swap_web_entry *insn_entry, unsigned int i)
> 
> A better name would be good, "handle" is a weaselword :-)  It is a
> static, so a shorter name is completely acceptable (say, one that
> wouldn't be acceptable with bigger than file scope).

Ok. How does convert_mem_insn() sound?
Note: "handle" is used as a prefix for other functions in rs6000-p8swap.cc (such as handle_special_swappables()).
> 
>> +  rtx_insn *insn = insn_entry[i].insn;
>> +  if (insn_entry[i].special_handling == SH_NOSWAP_LD)
>> +    permute_load (insn);
>> +  else if (insn_entry[i].special_handling == SH_NOSWAP_ST)
>> +    permute_store (insn);
> 
> Lose the "else"?  The compiler can do micro-optimisations a million
> times better than any user could.  Simpler, more readable (better
> understandable!) code is much preferred.
> 
>> +  /* Perform special handling for swappable insns that require it.
> 
> That is a completely contentless sentence :-(
> 

This line was present in the original code. This is not something I added.
Let me try to add some more comments to make the explanation better.

>> +     Note that special handling should be done only for those
>> +     swappable insns that are present in webs marked as requiring
>> +     special handling.  */
> 
> This one isn't much better.> 
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr106770.c
>> @@ -0,0 +1,21 @@
>> +/* { dg-do compile } */
> 
> This is the default, you do not need this.

ok.

> 
>> +/* { dg-require-effective-target powerpc_p8vector_ok } */
>> +/* { dg-options "-mdejagnu-cpu=power8 -O2 " } */
>> +/* The 2 xxpermdi instructions are generated by the two
>> +   calls to vec_promote() */
>> +/* { dg-final { scan-assembler-times "xxpermdi" 2 } } */
> 
> Please enclose in {}.  Use double quotes in Tcl only when tou want the
> interpolation they cause.  Default to using {} instead.

ok.

Regards,
Surya

> 
> So please fix those things, and write a better commit message.  Ideally
> the commit messsage will tell everything needed to understand the patch
> (so also to review the patch).  Maybe add examples where needed.  So
> reviewing the code in the patch should be an easy thing to do, after
> reading the commit message :-)
> 
> 
> Segher