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
