[Bug target/104005] New: Regression on arm+sve with -O2 -fPIC

[Bug target/104005] New: Regression on arm+sve with -O2 -fPIC

[gilles.gouaillardet at gmail dot com]

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

            Bug ID: 104005
           Summary: Regression on arm+sve with -O2 -fPIC
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gilles.gouaillardet at gmail dot com
  Target Milestone: ---

The GROMACS (2021.3) non regression test suite now hangs with the latest trunk
when shared libraries are used from -O2 when two or more thread-MPI are used. I
only tried 512 bit SVE vectors.

I ran a git bisect and it pointed to the following commit:

commit 526e1639aa76b0a8496b0dc3a3ff2c450229544e (refs/bisect/bad)
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Fri Nov 12 17:33:00 2021 +0000

    aarch64: Detect more consecutive MEMs


When investigating this issue, I found out that rebuilding the
src/gromacs/domdec/redistribute.cpp file **without** -fPIC is enough to avoid
the issue (a crash or a hang depending on the test cases).

FWIW, thread-MPI is a "fake" MPI implementation that used threads instead
processes. In this case, that means two threads. Then each thread enter its own
#pragma omp parallel section (and each thread-MPI thread has a unique OpenMP
thread)


The attached tarball includes preprocessed source (redistribute.cpp.i) and
assembly with -fPIC (redistribute.cpp.PIC.s) and without -fPIC
(redistribute.cpp.noPIC.s) for the commit mentioned above and the commit right
before.


Please let me know if this is enough for you to make sense of this bug,
otherwise I will provide instructions on how to build GROMACS from sources and
manually workaround the issue by "removing" the -fPIC flags for the
redistribute.cpp file.

[Bug target/104005] Regression on arm+sve with -O2 -fPIC

[rsandifo at gcc dot gnu.org]

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

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-01-14
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rsandifo at gcc dot gnu.org
     Ever confirmed|0                           |1
                 CC|                            |rsandifo at gcc dot gnu.org

--- Comment #1 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Thanks for the report.  I'll try to reproduce this next week.

[Bug target/104005] [12 Regression] Regression on arm+sve with -O2 -fPIC

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0
           Priority|P3                          |P1
            Summary|Regression on arm+sve with  |[12 Regression] Regression
                   |-O2 -fPIC                   |on arm+sve with -O2 -fPIC
           Keywords|                            |wrong-code

[Bug target/104005] [12 Regression] Regression on aarch64+sve with -O2 -fPIC

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>The attached tarball includes preprocessed source

I think it was too big when you tried to create the bug report (it is known
issue that bugzilla does not report the attachment fails when creating the bug
report). Was the tar ball compressed if not try xz to compress it and attach
that.

[Bug target/104005] [12 Regression] Regression on aarch64+sve with -O2 -fPIC since r12-5204

[gilles.gouaillardet at gmail dot com]

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

--- Comment #3 from Gilles Gouaillardet <gilles.gouaillardet at gmail dot com> ---
Created attachment 52210
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52210&action=edit
before/after (preprocessed sources + assembly)

[Bug target/104005] [12 Regression] Regression on aarch64+sve with -O2 -fPIC since r12-5204

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Gilles Gouaillardet from comment #3)
> Created attachment 52210 [details]
> before/after (preprocessed sources + assembly)

Thanks

[Bug target/104005] [12 Regression] Regression on aarch64+sve with -O2 -fPIC since r12-5204

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:38ec23fafb167ddfe840d7bb22b3e943d8a7d29e

commit r12-6669-g38ec23fafb167ddfe840d7bb22b3e943d8a7d29e
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Tue Jan 18 12:20:00 2022 +0000

    aarch64: Fix overly optimistic LDP/STP matching [PR104005]

    In g:526e1639aa76b0a8496b0dc3a3ff2c450229544e I'd added support
    for finding more consecutive MEMs.  However, the check was too
    eager, in that it matched MEM_REFs with the same base address
    even if that base address was an arbitrary SSA name.  This can
    give wrong results if a MEM_REF from one loop iteration is
    compared with a MEM_REF from another (e.g. after rtl unrolling).

    In principle, we could still accept MEM_REFs based on the same
    incoming SSA name, but there seems to be no out-of-the-box API
    for doing that.  Adding a new one at this stage in GCC 12 doesn't
    feel like a good risk/reward trade-off.

    This patch therefore restricts the MEM_EXPR comparison to base decls
    only, excluding all MEM_REFs.  It means we lose all the new STPs in
    the PR testcase but keep the ones in the original stp_1.c testcase.

    gcc/
            PR target/104005
            * config/aarch64/aarch64.cc (aarch64_check_consecutive_mems):
            When using MEM_EXPR, require the base to be a decl.

    gcc/testsuite/
            PR target/104005
            * gcc.target/aarch64/pr104005.c: New test.

[Bug target/104005] [12 Regression] Regression on aarch64+sve with -O2 -fPIC since r12-5204

[rsandifo at gcc dot gnu.org]

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

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

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

--- Comment #6 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Fixed on trunk.  Thanks for catching this before the GCC 12 release.

[Bug target/104005] [12 Regression] Regression on aarch64+sve with -O2 -fPIC since r12-5204

[gilles.gouaillardet at gmail dot com]

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

--- Comment #7 from Gilles Gouaillardet <gilles.gouaillardet at gmail dot com> ---
Thanks Richard for the quick fix!

Unfortunately, I just found an other (and much older) issue (ICE, only at
-Ofast) I reported at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104112