Hi Peter,
on 2024/8/21 21:14, Peter Bergner wrote:
> Our power8 swap optimization pass has some special handling for optimizing
> swaps of TImode variables. The test case reported in bugzilla uses a call
> to __atomic_compare_exchange, which introduces a variable of PTImode and
> that does not get the same treatment as TImode leading to wrong code
> generation. The simple fix is to treat PTImode identically to TImode.
Thanks for fixing, the fix looks good to me. The interesting rtxes, which
are optimized by p8swap and cause this issue, have both subreg on PTImode
and vector mode, the subreg special adjustment doesn't differentiate them
and replaces the one on PTImode unexpectedly, the fix makes the pass
consider it as not optimizable.
>
> This passed bootstrap and regtesting on powerpc64le-linux with no regressions.
> I also confirmed the testcase is correctly not run on -m32 BE and passes
> on -m64 BE.
>
> Ok for trunk?
>
>
> This is broken back to GCC 12, so ok for the releases branches after some
> bake-in time on trunk?
>
> Peter
>
>
> gcc/
> PR target/116415
> * config/rs6000/rs6000-p8swap.cc (rs6000_analyze_swaps): Handle PTImode
> identically to TImode.
>
> gcc/testsuite/
> PR target/116415
> * gcc.target/powerpc/pr116415.c: New test.
>
>
> diff --git a/gcc/config/rs6000/rs6000-p8swap.cc
> b/gcc/config/rs6000/rs6000-p8swap.cc
> index 639f477d782..15e44bb63a6 100644
> --- a/gcc/config/rs6000/rs6000-p8swap.cc
> +++ b/gcc/config/rs6000/rs6000-p8swap.cc
> @@ -2469,10 +2469,11 @@ rs6000_analyze_swaps (function *fun)
> mode = V4SImode;
> }
>
> - if (ALTIVEC_OR_VSX_VECTOR_MODE (mode) || mode == TImode)
> + if (ALTIVEC_OR_VSX_VECTOR_MODE (mode) || mode == TImode
> + || mode == PTImode)
Maybe we can introduce a macro to this file like
#define TI_OR_PTI_MODE(mode) ((mode) == TImode || (mode) == PTImode)
to simplify it a bit and people would likely notice and use this if they
want to add more handling for TImode in future (then it'll naturally
consider PTImode as well).
> {
> insn_entry[uid].is_relevant = 1;
> - if (mode == TImode || mode == V1TImode
> + if (mode == TImode || mode == PTImode || mode == V1TImode
> || FLOAT128_VECTOR_P (mode))
> insn_entry[uid].is_128_int = 1;
> if (DF_REF_INSN_INFO (mention))
> @@ -2497,10 +2498,11 @@ rs6000_analyze_swaps (function *fun)
> && ALTIVEC_OR_VSX_VECTOR_MODE (GET_MODE (SET_DEST (insn))))
> mode = GET_MODE (SET_DEST (insn));
>
> - if (ALTIVEC_OR_VSX_VECTOR_MODE (mode) || mode == TImode)
> + if (ALTIVEC_OR_VSX_VECTOR_MODE (mode) || mode == TImode
> + || mode == PTImode)
> {
> insn_entry[uid].is_relevant = 1;
> - if (mode == TImode || mode == V1TImode
> + if (mode == TImode || mode == PTImode || mode == V1TImode
> || FLOAT128_VECTOR_P (mode))
> insn_entry[uid].is_128_int = 1;
> if (DF_REF_INSN_INFO (mention))
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr116415.c
> b/gcc/testsuite/gcc.target/powerpc/pr116415.c
> new file mode 100644
> index 00000000000..5fad810ceb0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr116415.c
> @@ -0,0 +1,43 @@
> +/* { dg-do run } */
> +/* { dg-skip-if "" { powerpc*-*-darwin* } } */
Nit: This dg-skip-if line looks not necessary as p8vector_hw excludes
*-*-darwin*.
OK for trunk and all active release branches with/without these nits tweaked,
but please give others two days or so to comment, thanks!
BR,
Kewen
> +/* { dg-require-effective-target p8vector_hw } */
> +/* { dg-require-effective-target int128 } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power8" } */
> +
> +/* PR 116415: Verify our Power8 swap optimization pass doesn't incorrectly
> swap
> + PTImode values. They should be handled identically to TImode values. */
> +
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +
> +typedef union {
> + struct {
> + uint64_t a;
> + uint64_t b;
> + } t;
> + __uint128_t data;
> +} Value;
> +Value value, next;
> +
> +void
> +bug (Value *val, Value *nxt)
> +{
> + for (;;) {
> + nxt->t.a = val->t.a + 1;
> + nxt->t.b = val->t.b + 2;
> + if (__atomic_compare_exchange (&val->data, &val->data, &nxt->data,
> + 0, __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE))
> + break;
> + }
> +}
> +
> +int
> +main (void)
> +{
> + bug (&value, &next);
> + printf ("%lu %lu\n", value.t.a, value.t.b);
> + if (value.t.a != 1 || value.t.b != 2)
> + abort ();
> + return 0;
> +}
