Re: [PATCH v2,rs6000] Replace swap of a loaded vector constant with load of a swapped vector constant
On 26 September 2017 12:57:37 CEST, Segher Boessenkool
wrote:
>> --- 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?
Yea this b) is generally what is recommended see also
https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01358.html which reminds me that
the -fno-ident thing is still not upstream.
Or maybe we should have a -fno-file which suppresses .file like I proposed for
.ident. At least this would fix it once and for all.
>
>Or what else is it looking for? I bet b) holds anyway :-)
>
>Looks good except for those details.
>
>
>Segher
Re: [PATCH v2,rs6000] Replace swap of a loaded vector constant with load of a swapped vector constant
On Sep 26, 2017, at 5:57 AM, Segher Boessenkool
wrote:
>
>> +/* { 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?
Agreed, this looks like an unnecessary test for now. Changing to "xxswapd"
would future-proof the test in case we ever generated that. Agree with
Segher that it would be much better to have the tests have uniform naming.
No further comments from me; looks good.
Bill
Re: [PATCH v2,rs6000] Replace swap of a loaded vector constant with load of a swapped vector constant
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
[PATCH v2,rs6000] Replace swap of a loaded vector constant with load of a swapped vector constant
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.
This second version of the patch responds to feedback provided by Segher
Boessenkool, Bill Schmidt, and Pat Haugen. Thank you for the careful
reviews:
1. Revised comments in const_load_sequence_p function of rs6000-p8swap.c
2. Restructured nested if statements as a single if-statement with
compound condition in const_load_sequence_p function of
rs6000-p8swap.c
3. In replace_swapped_load_constant function of rs6000-p8swap.c,
replaced two FOR_EACH_INSN_INFO_USE macro expansions with
non-looping control structures.
4. Added comments and white space to replace_swapped_load_constant
function of rs6000-p8swap.c to improve readability.
5. Reordered handling of cases in replace_swapped_load_constant
function of rs6000-p8swap.c, moving V8HImode and V8HFmode
handling above V4SImode handling.
6. Replaced gcc_assert (0) with gcc_unreachable () in
replace_swapped_load_constant of rs6000-p8swap.c.
7. In rs6000_analyze_swaps function of rs6000-p8swap.c,
added requirement that !pass2_insn_entry[i].is_store
before calling const_load_sequence_p.
8. Removed unnecessary code blocks at end of
rs6000_analyze_swaps function of rs6000-p8swap.c.
9. Added 15 new tests to exercise different vector element sizes.
This patch has been bootstrapped and tested without regressions on
powerpc64le-unknown-linux (P8) and on powerpc-unknown-linux (P8,
big-endian, with both -m32 and -m64 target options).
Is this ok for trunk?
gcc/ChangeLog:
2017-09-25 Kelvin Nilsen
* config/rs6000/rs6000-p8swap.c (const_load_sequence_p): Revise
this function to return false if the definition used by the swap
instruction is artificial, or if the memory address from which the
constant value is loaded is not represented by a base address held
in a register or if the base address register is a frame or stack
pointer. Additionally, return false if the base address of the
loaded constant is a SYMBOL_REF but is not considered to be a
constant.
(replace_swapped_load_constant): New function.
(rs6000_analyze_swaps): Add a new pass to replace a swap of a
loaded constant vector with a load of a swapped constant vector.
gcc/testsuite/ChangeLog:
2017-09-25 Kelvin Nilsen
* 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.
Index: gcc/config/rs6000/rs6000-p8swap.c
===
--- gcc/config/rs6000/rs6000-p8swap.c (revision 252768)
+++ gcc/config/rs6000/rs6000-p8swap.c (working copy)
@@ -335,21 +335,26 @@ const_load_sequence_p (swap_web_entry *insn_entry,
const_rtx tocrel_base;
- /* Find the unique use in the swap and locate its def. If the def
- isn't unique, punt. */
struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
df_ref use;
FOR_EACH_INSN_INFO_USE (use, insn_info)
{
struct df_link *def_link = DF_REF_CHAIN (use);
- if (!def_link || def_link->next)
+
+ /* If there is no def or the def is artificial or there are
+multiple defs, punt. */
+ if (!def_link || !def_link->ref || DF_REF_IS_ARTIFICIAL (def_link->ref)
+ || def_link->next)
return false;
rtx def_insn = DF_REF_INSN (def_link->ref);
unsigned uid2 = INSN_UID (def_insn);
+ /* If this is not a load or is not a swap, return false */
if (!insn_entry[uid2].is_load || !insn_entry[uid2].is_swap)
return false;
+ /* If the source of the rtl def is not a set from memory, return
+false. */
rtx body = PATTERN (def_insn);
if (GET_CODE (body) != SET
|| GET_CODE (SET_SRC (body)) != VEC_SELECT
@@ -358,11 +363,17 @@ const_load_sequence_
