Re: [PATCH 0/4] Use pointer arithmetic for array references [PR102043]

[Richard Biener <richard.guenther@gmail.com>]

On Sat, Apr 16, 2022 at 6:57 PM Mikael Morin via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hello,
>
> this is a fix for PR102043, which is a wrong code bug caused by the
> middle-end concluding from array indexing that the array index is
> non-negative.  This is a wrong assumption for "reversed arrays",
> that is arrays whose descriptor makes accesses to the array from
> last element to first element.  More generally the wrong cases are
> arrays with a descriptor having a negative stride for at least one
> dimension.
>
> I have been trying to fix this by stopping the front-end from generating
> bogus code, by either stripping array-ness from descriptor data
> pointers, or by changing the initialization of data pointers to point
> to the first element in memory order instead of the first element in
> access order (which is the last in memory order for reversed arrays).
> Both ways are very impacting changes to the frontend and I haven’t
> been able to eliminate all the regressions in time using either way.
>
> However, Richi showed with a patch attached to the PR that array
> references are crucial for the problem to appear, and everything works
> if array indexing is replaced with pointer arithmetic.  This is
> much simpler and doesn’t imply invasive changes to the frontend.
>
> I have built on top of his patch to keep the array indexing in cases
> where the change to pointer arithmetic is not necessary, either because
> the array is not a fortran array with a descriptor, or because it’s
> known to be contiguous.  This has the benefit of reducing the churn
> in the dumped code patterns used in the testsuite.  It also avoids
> ICE regression such as interface_12.f90 or result_in_spec.f90, but
> I can’t exclude that those could be a real problem made latent.
>
> Patches 1 to 3 are preliminary changes to avoid regressions.  The main
> change is number 4, the last in the series.
>
> Regression tested on x86_64-pc-linux-gnu.  OK for master?

I've also tested the patch and built SPEC CPU 2017 successfully
on x86_64 with -Ofast -flto -march=znver2.  For 548.exchange2_r
I see a ~3% runtime regression caused by the change, the
other 6 Fortran using benchmarks show no runtime behavior change.
I have not analyzed the 548.exchange2_r regression (but confirmed
it with a 3-run).

That said, I believe this is the only reasonable thing to do for GCC 12,
all other options require invasive changes in the middle-end.

So OK from my side, I'm not familiar with the GFortran frontend enough
to review the changes besides the gfc_build_array_ref chage though.

Thanks,
Richard.


>
> Mikael Morin (4):
>   fortran: Pre-evaluate string pointers. [PR102043]
>   fortran: Update index extraction code. [PR102043]
>   fortran: Generate an array temporary reference [PR102043]
>   fortran: Use pointer arithmetic to index arrays [PR102043]
>
>  gcc/fortran/trans-array.cc                    |  60 +++++-
>  gcc/fortran/trans-expr.cc                     |   9 +-
>  gcc/fortran/trans-io.cc                       |  48 ++++-
>  gcc/fortran/trans.cc                          |  42 +++-
>  gcc/fortran/trans.h                           |   4 +-
>  .../gfortran.dg/array_reference_3.f90         | 195 ++++++++++++++++++
>  gcc/testsuite/gfortran.dg/c_loc_test_22.f90   |   4 +-
>  gcc/testsuite/gfortran.dg/dependency_49.f90   |   3 +-
>  gcc/testsuite/gfortran.dg/finalize_10.f90     |   2 +-
>  .../gfortran.dg/negative_stride_1.f90         |  25 +++
>  .../gfortran.dg/vector_subscript_8.f90        |  16 ++
>  .../gfortran.dg/vector_subscript_9.f90        |  21 ++
>  12 files changed, 401 insertions(+), 28 deletions(-)
>  create mode 100644 gcc/testsuite/gfortran.dg/array_reference_3.f90
>  create mode 100644 gcc/testsuite/gfortran.dg/negative_stride_1.f90
>  create mode 100644 gcc/testsuite/gfortran.dg/vector_subscript_8.f90
>  create mode 100644 gcc/testsuite/gfortran.dg/vector_subscript_9.f90
>
> --
> 2.35.1
>