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!)
> 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.
> + /* 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)
> +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.
> +/* 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).
> + 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 :-(
> + 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.
> +/* { 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.
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
