Hi Kelvin,
On Mon, Sep 25, 2017 at 04:11:32PM -0600, Kelvin Nilsen wrote:
> On Power8 little endian, two instructions are needed to load from the
> natural in-memory representation of a vector into a vector register: a
> load followed by a swap. When the vector value to be loaded is a
> constant, more efficient code can be achieved by swapping the
> representation of the constant in memory so that only a load instruction
> is required.
> * gcc.target/powerpc/swaps-p8-28.c: New test.
> * gcc.target/powerpc/swaps-p8-29.c: New test.
> * gcc.target/powerpc/swaps-p8-31.c: New test.
> * gcc.target/powerpc/swaps-p8-32.c: New test.
> * gcc.target/powerpc/swaps-p8-34.c: New test.
> * gcc.target/powerpc/swaps-p8-35.c: New test.
> * gcc.target/powerpc/swaps-p8-37.c: New test.
> * gcc.target/powerpc/swaps-p8-38.c: New test.
> * gcc.target/powerpc/swaps-p8-40.c: New test.
> * gcc.target/powerpc/swaps-p8-41.c: New test.
> * gcc.target/powerpc/swaps-p8-43.c: New test.
> * gcc.target/powerpc/swaps-p8-44.c: New test.
> * gcc.target/powerpc/swps-p8-30.c: New test.
> * gcc.target/powerpc/swps-p8-33.c: New test.
> * gcc.target/powerpc/swps-p8-36.c: New test.
> * gcc.target/powerpc/swps-p8-39.c: New test.
> * gcc.target/powerpc/swps-p8-42.c: New test.
> * gcc.target/powerpc/swps-p8-45.c: New test.
I think you want to name those "swps" files "swaps" as well? (See below).
> + /* If this is not a load or is not a swap, return false */
End the sentence with a dot (and space space) please.
> + /* Constants held on the stack are not "true" constants
> + because their values are not part of the static load
> + image. If this constant's base reference is a stack
> + or frame pointer, it is seen as an artificial
> + reference. */
Dot space space.
> +static void
> +replace_swapped_load_constant (swap_web_entry *insn_entry, rtx swap_insn)
> +{
> + /* Find the load. */
> + struct df_insn_info *insn_info = DF_INSN_INFO_GET (swap_insn);
> + rtx_insn *load_insn = 0;
Don't initialise this (you set it a few lines later :-) )
> + df_ref use = DF_INSN_INFO_USES (insn_info);
> + gcc_assert (use);
> +
> + struct df_link *def_link = DF_REF_CHAIN (use);
> + gcc_assert (def_link && !def_link->next);
> +
> + load_insn = DF_REF_INSN (def_link->ref);
> + gcc_assert (load_insn);
You can remove most of these asserts btw; if e.g. the first one would
fail, the very next line would ICE anyway. The ->next test is probably
useful; if you don have useless asserts the useful ones stand out more ;-)
> + else if ((mode == V8HImode)
> +#ifdef HAVE_V8HFmode
> + || (mode == V8HFmode)
> +#endif
> + )
Hrm. So rs6000-modes.def claims it is creating V8HFmode:
VECTOR_MODES (FLOAT, 16); /* V8HF V4SF V2DF */
but VECTOR_MODES does not do that, because we have no HFmode. Surprising.
Looks like a bug even.
Maybe we want to delete the #ifdef later; this code is fine until then.
> --- gcc/testsuite/gcc.target/powerpc/swaps-p8-28.c (revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/swaps-p8-28.c (working copy)
> @@ -0,0 +1,29 @@
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-do run { target { powerpc*-*-* } } } */
> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } {
> "-mcpu=power8" } } */
> +/* { dg-options "-mcpu=power8 -O3 " } */
Run tests need to check p8vector_hw instead of powerpc_p8vector_ok, or they
will crash on older systems.
> --- gcc/testsuite/gcc.target/powerpc/swps-p8-36.c (revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/swps-p8-36.c (working copy)
> @@ -0,0 +1,31 @@
> +/* This file's name was changed from swaps-p8-36.c so that the
> + assembler search for "not swap" would not get a false
> + positive on the name of the file. */
Oh.
> +/* { dg-final { scan-assembler-not "swap" } } */
So what is this really testing for? xxswapd? But a) we never generate
that, and b) you could use a better regex?
Or what else is it looking for? I bet b) holds anyway :-)
Looks good except for those details.
Segher
