| Message ID | 20220416165618.236666-1-mikael@gcc.gnu.org |
|---|---|
| Headers | show |
| Series | Use pointer arithmetic for array references [PR102043] | expand |
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 >
Ping for the four patches starting at https://gcc.gnu.org/pipermail/fortran/2022-April/057759.html : https://gcc.gnu.org/pipermail/fortran/2022-April/057757.html https://gcc.gnu.org/pipermail/fortran/2022-April/057760.html https://gcc.gnu.org/pipermail/fortran/2022-April/057758.html https://gcc.gnu.org/pipermail/fortran/2022-April/057761.html Richi accepted the general direction and the middle-end interaction. I need a fortran frontend ack as well.
Hi Mikael, > Ping for the four patches starting at > https://gcc.gnu.org/pipermail/fortran/2022-April/057759.html : > https://gcc.gnu.org/pipermail/fortran/2022-April/057757.html > https://gcc.gnu.org/pipermail/fortran/2022-April/057760.html > https://gcc.gnu.org/pipermail/fortran/2022-April/057758.html > https://gcc.gnu.org/pipermail/fortran/2022-April/057761.html > > Richi accepted the general direction and the middle-end interaction. > I need a fortran frontend ack as well. Looks good to me. Thanks a lot for taking this on! This would have been a serious regression if released with gcc 12. Best regards Thomas
Yes, Thank you Mikael! On 4/22/22 6:59 AM, Thomas Koenig via Fortran wrote: > > Hi Mikael, > >> Ping for the four patches starting at >> https://gcc.gnu.org/pipermail/fortran/2022-April/057759.html : >> https://gcc.gnu.org/pipermail/fortran/2022-April/057757.html >> https://gcc.gnu.org/pipermail/fortran/2022-April/057760.html >> https://gcc.gnu.org/pipermail/fortran/2022-April/057758.html >> https://gcc.gnu.org/pipermail/fortran/2022-April/057761.html >> >> Richi accepted the general direction and the middle-end interaction. >> I need a fortran frontend ack as well. > > Looks good to me. > > Thanks a lot for taking this on! This would have been a serious > regression if released with gcc 12. > > Best regards > > Thomas
> From: Thomas Koenig via Gcc-patches <gcc-patches@gcc.gnu.org> > Date: Fri, 22 Apr 2022 15:59:45 +0200 > Hi Mikael, > > > Ping for the four patches starting at > > https://gcc.gnu.org/pipermail/fortran/2022-April/057759.html : > > https://gcc.gnu.org/pipermail/fortran/2022-April/057757.html > > https://gcc.gnu.org/pipermail/fortran/2022-April/057760.html > > https://gcc.gnu.org/pipermail/fortran/2022-April/057758.html > > https://gcc.gnu.org/pipermail/fortran/2022-April/057761.html > > > > Richi accepted the general direction and the middle-end interaction. > > I need a fortran frontend ack as well. > > Looks good to me. > > Thanks a lot for taking this on! This would have been a serious > regression if released with gcc 12. > > Best regards > > Thomas These, or specifically r12-8227-g89ca0fffa48b79, "fortran: Pre-evaluate string pointers. [PR102043]" have further exposed (the issue existed before but now fails for more platforms) PR78054 "gfortran.dg/pr70673.f90 FAILs at -O0", at least for cris-elf and apparently also s390x-ibm-linux-gnu. In the PR it is mentioned that running the test through valgrind shows invalid accesses also on x86_64-linux-gnu. Could it be that the test-case is invalid and has undefined behavior? I don't know fortran so I can't tell. That exact commit causing a regression for s390x is somewhat an assumption based on posted date and testresults, as the s390x results don't include a git version. (@Stefansf: I'm referring to https://gcc.gnu.org/pipermail/gcc-testresults/2022-April/760060.html https://gcc.gnu.org/pipermail/gcc-testresults/2022-April/760137.html Perhaps that tester isn't using the contrib/gcc_update and contrib/test_summary scripts, thus no LAST_UPDATED included?) brgds, H-P
On 26.04.22 16:40, Hans-Peter Nilsson wrote: > These, or specifically r12-8227-g89ca0fffa48b79, "fortran: > Pre-evaluate string pointers. [PR102043]" have further > exposed (the issue existed before but now fails for more > platforms) PR78054 "gfortran.dg/pr70673.f90 FAILs at -O0", > at least for cris-elf and apparently also > s390x-ibm-linux-gnu. > > In the PR it is mentioned that running the test through > valgrind shows invalid accesses also on x86_64-linux-gnu. > Could it be that the test-case is invalid and has undefined > behavior? I don't know fortran so I can't tell. You're right. I just looked at the test case and can confirm what Steve Kargl observed in the PR, it is in fact invalid. You cannot do a = a after deallocating a. I've assigned the PR to myself and will commit the change to dg-do compile soon (unless anybody objects). Best regards Thomas
On Tue, Apr 26, 2022 at 04:40:33PM +0200, Hans-Peter Nilsson wrote: > > From: Thomas Koenig via Gcc-patches <gcc-patches@gcc.gnu.org> > > Date: Fri, 22 Apr 2022 15:59:45 +0200 > > > Hi Mikael, > > > > > Ping for the four patches starting at > > > https://gcc.gnu.org/pipermail/fortran/2022-April/057759.html : > > > https://gcc.gnu.org/pipermail/fortran/2022-April/057757.html > > > https://gcc.gnu.org/pipermail/fortran/2022-April/057760.html > > > https://gcc.gnu.org/pipermail/fortran/2022-April/057758.html > > > https://gcc.gnu.org/pipermail/fortran/2022-April/057761.html > > > > > > Richi accepted the general direction and the middle-end interaction. > > > I need a fortran frontend ack as well. > > > > Looks good to me. > > > > Thanks a lot for taking this on! This would have been a serious > > regression if released with gcc 12. > > > > Best regards > > > > Thomas > > These, or specifically r12-8227-g89ca0fffa48b79, "fortran: > Pre-evaluate string pointers. [PR102043]" have further > exposed (the issue existed before but now fails for more > platforms) PR78054 "gfortran.dg/pr70673.f90 FAILs at -O0", > at least for cris-elf and apparently also > s390x-ibm-linux-gnu. > > In the PR it is mentioned that running the test through > valgrind shows invalid accesses also on x86_64-linux-gnu. > Could it be that the test-case is invalid and has undefined > behavior? I don't know fortran so I can't tell. > > That exact commit causing a regression for s390x is somewhat > an assumption based on posted date and testresults, as the > s390x results don't include a git version. (@Stefansf: I'm > referring to > https://gcc.gnu.org/pipermail/gcc-testresults/2022-April/760060.html > https://gcc.gnu.org/pipermail/gcc-testresults/2022-April/760137.html > Perhaps that tester isn't using the contrib/gcc_update and > contrib/test_summary scripts, thus no LAST_UPDATED > included?) Indeed the reports don't include a git commit id. We are using both scripts. However, since the git repository is setup differently in our case, we had been using `gcc_update --touch` only. Thus the files LAST_UPDATED as well as gcc/REVISION were not created. I changed that such that both are created, now. Thanks for letting me know! Cheers, Stefan