[Bug tree-optimization/109230] New: [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[Bug tree-optimization/109230] New: [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

            Bug ID: 109230
           Summary: [13 Regression] Maybe wrong code for opus package on
                    aarch64 since r13-4122-g1bc7efa948f751
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: tnfchris at gcc dot gnu.org
  Target Milestone: ---
              Host: aarch64-linux-gnu
            Target: aarch64-linux-gnu

The package build fails here:
https://build.opensuse.org/package/live_build_log/openSUSE:Factory:ARM/opus/standard/aarch64

[  214s] FAIL: celt/tests/test_unit_mdct
[  214s] ===============================
[  214s] 
[  214s] nfft=32 inverse=0,snr = 17.104167
[  214s] ** poor snr: 17.104167 **
[  214s] nfft=32 inverse=1,snr = 20.102683
[  214s] ** poor snr: 20.102683 **
[  214s] nfft=256 inverse=0,snr = 138.489354
[  214s] nfft=256 inverse=1,snr = 137.974696
[  214s] nfft=512 inverse=0,snr = 7.811854
[  214s] ** poor snr: 7.811854 **
[  214s] nfft=512 inverse=1,snr = 7.850219
[  214s] ** poor snr: 7.850219 **
[  214s] nfft=1024 inverse=0,snr = 136.654582
[  214s] nfft=1024 inverse=1,snr = 136.537410
[  214s] nfft=2048 inverse=0,snr = 9.257344
[  214s] ** poor snr: 9.257344 **
[  214s] nfft=2048 inverse=1,snr = 10.914094
[  214s] ** poor snr: 10.914094 **
[  214s] nfft=36 inverse=0,snr = 137.555495
[  214s] nfft=36 inverse=1,snr = 139.119054
[  214s] nfft=40 inverse=0,snr = 134.266617
[  214s] nfft=40 inverse=1,snr = 137.088788
[  214s] nfft=60 inverse=0,snr = 135.700513
[  214s] nfft=60 inverse=1,snr = 139.498130
[  214s] nfft=120 inverse=0,snr = 136.557198
[  214s] nfft=120 inverse=1,snr = 139.409077
[  214s] nfft=240 inverse=0,snr = 136.620940
[  214s] nfft=240 inverse=1,snr = 137.494083
[  214s] nfft=480 inverse=0,snr = 7.290223
[  214s] ** poor snr: 7.290223 **
[  214s] nfft=480 inverse=1,snr = 10.126481
[  214s] ** poor snr: 10.126481 **
[  214s] nfft=960 inverse=0,snr = 136.037327
[  214s] nfft=960 inverse=1,snr = 137.196856
[  214s] nfft=1920 inverse=0,snr = 8.653704
[  214s] ** poor snr: 8.653704 **
[  214s] nfft=1920 inverse=1,snr = 9.472643
[  214s] ** poor snr: 9.472643 **
[  214s] FAIL celt/tests/test_unit_mdct (exit status: 1)

and it's since the revision r13-4122-g1bc7efa948f751.

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[tnfchris at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #1 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
That patch only fixed the bootstrap, in any case I'm on holidays so have asked
someone else to look.

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
And the same happens for glm package:
https://build.opensuse.org/package/live_build_log/openSUSE:Factory:ARM/glm/standard/aarch64

[   95s] The following tests FAILED:
[   95s]        168 - test-gtx_dual_quaternion (Failed)

That also started with the mentioned revision.

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.0

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[avieira at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

avieira at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |avieira at gcc dot gnu.org

--- Comment #3 from avieira at gcc dot gnu.org ---
Hi Martin, what options do you build these tests with?

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #4 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to avieira from comment #3)
> Hi Martin, what options do you build these tests with?

Clicking on the link for the glm package (and then "download logfile") shows
for instance the following (I have not checked whether a certain fine is
compiled differently):

/usr/bin/c++  -I/home/abuild/rpmbuild/BUILD/glm-0.9.9.8
-mbranch-protection=standard -O2
-Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3
-fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables
-fstack-clash-protection -Werror=return-type -flto=auto -g -fPIC
-fno-strict-aliasing -O2 -g -DNDEBUG -O2 -Wno-long-long -MD

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #5 from Martin Liška <marxin at gcc dot gnu.org> ---
Steps to reproduce:

$ wget https://archive.mozilla.org/pub/opus/opus-1.3.1.tar.gz
$ tar xvzf opus-1.3.1.tar.gz
$ cd opus-1.3.1/
$ ./configure
$ make -j32 && make -j32 check

So it fails even with default options that are:
-g -O2 -fvisibility=hidden -D_FORTIFY_SOURCE=2 -W -Wall -Wextra -Wcast-align
-Wnested-externs -Wshadow -Wstrict-prototypes

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[avieira at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #6 from avieira at gcc dot gnu.org ---
Thanks!

My initial investigation has lead me to think the change is being caused at
vrp2, which is the only time the pattern gets triggered with -O2, the tree
before the pass (at the place where the transformation happens):

  vect__83.466_787 = VEC_PERM_EXPR <vect__83.456_796, vect__83.456_796, { 1, 1
}>;
  vect__87.467_786 = vect__81.462_791 * vect__83.466_787;
  vect__91.469_784 = vect__84.458_794 - vect__87.467_786;
  vect__88.468_785 = vect__84.458_794 + vect__87.467_786;
  _783 = VEC_PERM_EXPR <vect__88.468_785, vect__91.469_784, { 0, 3 }>;
 ...
  vect__96.470_782 = vect__95.450_800 - _783;

after the pass:
  vect__83.466_787 = VEC_PERM_EXPR <vect__83.456_796, vect__83.456_796, { 1, 1
}>;
  vect__87.467_786 = vect__83.466_787 * vect__81.462_791;
  vect__91.469_784 = vect__84.458_794 - vect__87.467_786;
  vect__88.468_785 = vect__87.467_786 + vect__84.458_794;
  _756 = VIEW_CONVERT_EXPR<double>(vect__87.467_786);
  _755 = -_756;
  _739 = VIEW_CONVERT_EXPR<vector(2) float>(_755);
  _783 = _739 + vect__84.458_794;
...
  vect__96.470_782 = vect__95.450_800 - _783;

So before we had:
_783 = the first element of vect_88 and the second element of vect__91
these are respectively
vect__88 = vect__84 + vect__87
vect__91 = vect__84 - vect__87
so _783 = {vect__84[0] + vect__87[0], vect__84[1] - vect__87[1]}

after the pass
_783 = _739 + vect__84
This is where I don't know if I'm reading the optimization correctly, but it
says all 'even' lanes are negated, does that mean we end up with:
_739 = { -vect__87[0] , vect__87[1]}
if so then that's why we have a wrong result as you want to negate lane 1 not
0.  Otherwise if lane 1 is the one that gets negated then it should be OK as
you'd get:
so _783 = { vect__87[0] + vect__84[0], -vect__87[1] + vect__84[1] }
Now obviously that's assuming -a + b == b - a (not sure if that's true with
floating point errors etc)

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #7 from Martin Liška <marxin at gcc dot gnu.org> ---
I've just isolated that to celt/kiss_fft.c, can you confirm the change comes
from the file?

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #8 from Martin Liška <marxin at gcc dot gnu.org> ---
Where I see the following change in optimized dump:

   float _423;
   float _424;
   float _425;
+  vector(4) float _442;
+  vector(2) double _443;
   void * _742;
   unsigned int _743;
   const struct kiss_fft_state * _744;
@@ -383,6 +385,7 @@
   long unsigned int _914;
   unsigned long _919;
   void * _927;
+  vector(2) double _930;
   struct kiss_fft_cpx * _935;
   sizetype _936;
   unsigned long _941;
@@ -530,13 +533,14 @@
   _89 = _88 * 7.07106769084930419921875e-1;
   _866 = {_76, _75, _86, _89};
   _868 = {_74, _78, _86, _89};
-  vect__93.89_859 = vect__56.86_870 - _866;
   vect__93.89_858 = vect_t_r_54.81_878 - _867;
   vect__93.89_857 = vect__56.87_869 - _868;
   vect__60.88_864 = _865 + vect__56.85_871;
-  vect__60.88_863 = _866 + vect__56.86_870;
   vect__60.88_861 = _868 + vect__56.87_869;
-  _855 = VEC_PERM_EXPR <vect__60.88_863, vect__93.89_859, { 0, 5, 2, 3 }>;
+  _930 = VIEW_CONVERT_EXPR<vector(2) double>(_866);
+  _443 = -_930;
+  _442 = VIEW_CONVERT_EXPR<vector(4) float>(_443);
+  _855 = _442 + vect__56.86_870;
   _853 = VEC_PERM_EXPR <vect__93.89_857, vect__60.88_861, { 0, 5, 2, 3 }>;
   MEM <vector(4) float> [(float *)Fout_512] = vect__60.88_864;
   MEM <vector(4) float> [(float *)Fout_512 + 16B] = _855;

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[avieira at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #9 from avieira at gcc dot gnu.org ---
Hmm I was seeing the change in opus_ifft but that does look like different
codegen :/ I might not be looking at the right thing.

That transformation looks definitely wrong though as the selection selects 3
values from the first vector (which is the result of the plus), and the fneg
would negate 2 values right?

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So are we talking about
typedef float V __attribute__((vector_size (16)));
typedef int VI __attribute__((vector_size (16)));

V
foo (V x, V y)
{
  V a = x - y;
  V b = y + x;
  return __builtin_shuffle (b, a, (VI) { 0, 5, 2, 3 });
}
then?

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[tnfchris at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #11 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
Neither of those vec_perms are valid targets for this optimization.

It looks like sel.series_p is not doing what I expected. It's matching even
elements and ignoring the odd ones.

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Full self-contained testcase, PASSes with -O0, FAILs on aarch64-linux with -O2:
#if __SIZEOF_FLOAT__ == __SIZEOF_INT__
typedef float V __attribute__((vector_size (4 * sizeof (float))));
typedef int VI __attribute__((vector_size (4 * sizeof (float))));

__attribute__((noipa)) V
foo (V x, V y)
{
  V a = x - y;
  V b = y + x;
  return __builtin_shuffle (b, a, (VI) { 0, 5, 2, 3 });
}

int
main ()
{
  V a = (V) { 1.0f, 2.0f, 3.0f, 4.0f };
  V b = (V) { 8.0f, 9.0f, 10.0f, 11.0f };
  V c = foo (a, b);
  if (c[0] != 9.0f || c[1] != -7.0f || c[2] != 13.0f || c[3] != 15.0f)
    __builtin_abort ();
}
#else
int
main ()
{
}
#endif

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #13 from Martin Liška <marxin at gcc dot gnu.org> ---
Jakub, you are too fast! Anyway, nice test-case. So far I was able to come to
something semi-reduced:

typedef struct {
  float r;
  float i;
} kiss_fft_cpx;

kiss_fft_cpx *kf_bfly2_Fout, *opus_fft_impl_Fout2;
int opus_fft_impl_tw;

void
opus_fft_impl() {
  float t_0 = (0 + opus_fft_impl_Fout2[1].i) * opus_fft_impl_tw;
  float t_1 = (opus_fft_impl_Fout2[1].i - opus_fft_impl_Fout2[1].r) *
opus_fft_impl_tw;
  opus_fft_impl_Fout2[1].r = kf_bfly2_Fout[1].r - t_0;
  opus_fft_impl_Fout2[1].i = kf_bfly2_Fout[1].i - t_1;
  kf_bfly2_Fout[1].r += t_0;
  kf_bfly2_Fout[1].i += t_1;
}

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think series_p clearly documents that:
/* Return true if index OUT_BASE + I * OUT_STEP selects input
   element IN_BASE + I * IN_STEP.  For example, the call to test
   whether a permute reverses a vector of N elements would be:

     series_p (0, 1, N - 1, -1)

   which would return true for { N - 1, N - 2, N - 3, ... }.
   The calls to test for an interleaving of elements starting
   at N1 and N2 would be:

     series_p (0, 2, N1, 1) && series_p (1, 2, N2, 1).

   which would return true for { N1, N2, N1 + 1, N2 + 1, ... }.  */

bool
vec_perm_indices::series_p (unsigned int out_base, unsigned int out_step,
                            element_type in_base, element_type in_step) const

So, if you want to catch permutation { 0, nelts, 2, nelts + 2, 4, nelts + 4,
... }
then I think you need sel.series_p (0, 2, 0, 2) && sel.series_p (1, 2, nelts +
1, 2).

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-03-21
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I'll handle this.

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
--- gcc/match.pd.jj     2023-02-18 12:38:30.967022708 +0100
+++ gcc/match.pd        2023-03-21 17:24:54.964665797 +0100
@@ -8096,6 +8096,7 @@ and,
        scalar_mode inner_mode = GET_MODE_INNER (vec_mode);
      }
      (if (sel.series_p (0, 2, 0, 2)
+         && sel.series_p (1, 2, nelts + 1, 2)
          && GET_MODE_2XWIDER_MODE (inner_mode).exists (&wide_elt_mode)
          && multiple_p (GET_MODE_NUNITS (vec_mode), 2, &wide_nunits)
          && related_vector_mode (vec_mode, wide_elt_mode,
seems to indeed fix this.

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54725
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54725&action=edit
gcc13-pr109230.patch

Full untested patch.

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

--- Comment #18 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:07fc3491260e6b5d261433c977a4e069f5ab40c1

commit r13-6866-g07fc3491260e6b5d261433c977a4e069f5ab40c1
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sun Mar 26 20:17:00 2023 +0200

    match.pd: Fix up fneg/fadd simplification [PR109230]

    The following testcase is miscompiled on aarch64-linux.  match.pd
    has a simplification for addsub, where it negates one of the vectors
    in twice as large floating point element vector (effectively negating every
    other element) and then doing addition.
    But a requirement for that is that the permutation picks the right
elements,
    in particular 0, nelts+1, 2, nelts+3, 4, nelts+5, ...
    The pattern tests this with sel.series_p (0, 2, 0, 2) check, which as
    documented verifies that the even elements of the permutation mask are
    identity, but doesn't say anything about the others.
    The following patch fixes it by also checking that the odd elements
    start at nelts + 1 with the same step of 2.

    2023-03-26  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/109230
            * match.pd (fneg/fadd simplify): Verify also odd permutation
indexes.

            * gcc.dg/pr109230.c: New test.

[Bug tree-optimization/109230] [13 Regression] Maybe wrong code for opus package on aarch64 since r13-4122-g1bc7efa948f751

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109230

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.