Hi Ke Wen,
On Tue, Sep 15, 2020 at 02:40:38PM +0800, Kewen.Lin wrote:
> >> * config/rs6000/rs6000-p8swap.c (insn_rtx_pair_t): New type.
> >
> > Please don't do that. The "first" and "second" are completely
> > meaningless. Also, keeping it separate arrays can very well result in
> > better machine code, and certainly makes easier to read source code.
>
> OK, use separate arrays instead. Here the first is the AND rtx_insn
> while the second is its fully-expanded rtx, I thought it's better to
> bundle them together before, make_pair is an easy way for that.
Easy to write, hard to read.
> >> - rtx and_operation = 0;
> >> + rtx and_operation = NULL_RTX;
> >
> > Don't change code randomly (to something arguably worse, even).
>
> Done. I may think too much and thought NULL_RTX may be preferred
> since it has the potential to be changed by defining it as nullptr
> in the current C++11 context.
In all normal contexts you can use any null pointer constant (like 0) in
C++ as well.
> * config/rs6000/rs6000-p8swap.c (find_alignment_op): Adjust to
> support multiple defintions which are all AND operations with
> the mask -16B.
> (recombine_lvx_pattern): Adjust to handle multiple AND operations
> from find_alignment_op.
> (recombine_stvx_pattern): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/pr97019.c: New test.
> + gcc_assert (and_insns.length () == and_ops.length ());
+1. Thanks.
> + for (unsigned i = 0; i < and_insns.length (); ++i)
"i++" is used more often, is more traditional.
> + {
> + /* However, first we must be sure that we make the
> + base register from the AND operation available
> + in case the register has been overwritten. Copy
> + the base register to a new pseudo and use that
> + as the base register of the AND operation in
> + the new LVX instruction. */
The swaps pass runs very early (first thing after expand really), so
this is okay.
(Not that this is new code anyway, heh.)
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr97019.c
> @@ -0,0 +1,82 @@
> +/* This issue can only exist on little-endian P8 targets, since
> + the built-in functions vec_ld/vec_st will use lxvd2x/stxvd2x
> + (P8 big-endian) or lxv/stxv (P9 and later). */
> +/* { dg-do compile { target { powerpc_p8vector_ok && le } } } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power8" } */
Do you need to test for LE? If not, just always run it? If it works,
it works, it doesn't matter that you do not expect it to ever fail (we
do not really expect *any* test we have to ever fail *anywhere*, heh).
> +/* { dg-final { scan-assembler-not "rldicr\[ \t\]+\[0-9\]+,\[0-9\]+,0,59" }
> } */
Please use {} quotes, and \s and \d.
You can also use {(?n)rldicr.*,0,59} since (?n) makes . not match
newlines anymore.
Okay for trunk with or without those suggestions. Thanks!
Segher
