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 >
