From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x531.google.com (mail-ed1-x531.google.com [IPv6:2a00:1450:4864:20::531]) by sourceware.org (Postfix) with ESMTPS id 388483858C83; Tue, 19 Apr 2022 15:05:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 388483858C83 Received: by mail-ed1-x531.google.com with SMTP id c6so21655199edn.8; Tue, 19 Apr 2022 08:05:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=HVXBiYEknrUEI9wge9XBfDrw+qb67P9RXEZMkBbWP4Y=; b=tc+Xpp50gCQFEfOgNRFNlyBc2WGiRLAT29p5JsSd4lkBnHT3azxMA7Wljvm4vOAgkj khLUiCl8FPgnRqucpCLFJCpXhAA4tKLf5Gy1iP4VvodKUEkousN57D/5XOp34khWeaA1 i3HTCAjJ5yGRYnUVN6U7Yuksn83xILlxF3NBxFbVTEQ3Lw3XkMVK7h8xI4Qus1NNKmDh 7sxYpO7UDx+GhlcLNkUf48/YCj2pb4FLAGr5Xm42YIKBpvWDlW0YyFqXchR4U28epuXj nDWydQ0FGLYml6Hi7IfE3qftlE5/mv4o1/wuFffwv6xU0FIUgqAyXW//tjQgAM/XExD2 MDKA== X-Gm-Message-State: AOAM533ILLbDD3ZqVkz9OmZ41xVD1UtOaZXvojzAoYZ7t+U5khcssWdv SpeEXM4IIn8s3NDf9j7UYyr/ot9+PMC7eLQYFA48I9kvjhs= X-Google-Smtp-Source: ABdhPJwFihgx84XQ49rTCgdlEXWxkeKElUyvmg9T0LZgD5KQfNkNZc1fB35FfTm4gK24REad8N9RcQ8X2AUOKf3DbwQ= X-Received: by 2002:a05:6402:845:b0:421:fcb5:55de with SMTP id b5-20020a056402084500b00421fcb555demr18125863edz.124.1650380730661; Tue, 19 Apr 2022 08:05:30 -0700 (PDT) MIME-Version: 1.0 References: <20220416165618.236666-1-mikael@gcc.gnu.org> In-Reply-To: <20220416165618.236666-1-mikael@gcc.gnu.org> From: Richard Biener Date: Tue, 19 Apr 2022 17:05:19 +0200 Message-ID: Subject: Re: [PATCH 0/4] Use pointer arithmetic for array references [PR102043] To: Mikael Morin Cc: GCC Patches , "fortran@gcc.gnu.org" , Richard Biener Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: fortran@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Fortran mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Apr 2022 15:05:34 -0000 On Sat, Apr 16, 2022 at 6:57 PM Mikael Morin via Gcc-patches 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=E2=80=99= 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=E2=80=99t 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=E2=80= =99s > 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=E2=80=99t 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=3Dznver2. 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 >