Message ID | 20220918201545.453296-1-mikael@gcc.gnu.org |
---|---|
Headers | show |
Series | fortran: clobber fixes [PR41453] | expand |
Le 22/09/2022 à 22:42, Harald Anlauf via Fortran a écrit : > Hi Mikael, > > thanks for this impressive series of patches. > > Am 18.09.22 um 22:15 schrieb Mikael Morin via Fortran: >> The first patch is a refactoring moving the clobber generation in >> gfc_conv_procedure_call where it feels more appropriate. >> The second patch is a fix for the ICE originally motivating my work >> on this topic. >> The third patch is a fix for some wrong code issue discovered with an >> earlier version of this series. > > This LGTM. It also fixes a regression introduced with r9-3030 :-) > If you think that this set (1-3) is backportable, feel free to do so. > Yes, 2 and 3 are worth backporting, I will see how dependent they are on 1. >> The following patches are gradual condition loosenings to enable clobber >> generation in more and more cases. > > Patches 4-8 all look fine. Since they address missed optimizations, > they are probably something for mainline. > > I was wondering if you could add a test for the change in patch 7 > addressing the clobber generation for an associate-name, e.g. by > adding to testcase intent_optimize_7.f90 near the end: > > associate (av => ct) > av = 111222333 > call foo(av) > end associate > if (ct /= 42) stop 3 > > plus the adjustments in the patterns. > Indeed, I didn't add a test because there was one already, but the existing test hasn't the check for clobber generation and store removal. I prefer to create a new test though, so that the patch and the test come together, and the test for patch 8 is not encumbered with unrelated stuff. By the way, the same could be said about patch 6. I will create a test for that one as well. > Regarding patch 9, I do not see anything wrong with it, but then > independent eyes might see more. I think it is ok for mainline > as is. > >> Each patch has been tested through an incremental bootstrap and a >> partial testsuite run on fortran *intent* tests, and the whole lot has >> been run through the full fortran regression testsuite. >> OK for master? > > Yes. > Thanks for the review.
Le 23/09/2022 à 09:54, Mikael Morin a écrit : > Le 22/09/2022 à 22:42, Harald Anlauf via Fortran a écrit : >> I was wondering if you could add a test for the change in patch 7 >> addressing the clobber generation for an associate-name, e.g. by >> adding to testcase intent_optimize_7.f90 near the end: >> >> associate (av => ct) >> av = 111222333 >> call foo(av) >> end associate >> if (ct /= 42) stop 3 >> >> plus the adjustments in the patterns. >> > Indeed, I didn't add a test because there was one already, but the > existing test hasn't the check for clobber generation and store removal. > I prefer to create a new test though, so that the patch and the test > come together, and the test for patch 8 is not encumbered with unrelated > stuff. > > By the way, the same could be said about patch 6. > I will create a test for that one as well. > Patches pushed: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=77bbf69d2981dafc2ef3e59bfbefb645d88bab9d Changes from v2: - patches 6 and 7: A test for each has been added. - patches 8 and 9: The tests have been renumbered. - patches 6 and 7: The PR number used in the subject line has been changed, from the different regression PRs to the one optimization PR. - patches 5 and 8: The commit message has been modified: the commit the patch partly reverts is mentioned, and the associated PR number as well. - patch 7: The regression PR number this refers to has been changed.
Le 23/09/2022 à 09:54, Mikael Morin a écrit : > Le 22/09/2022 à 22:42, Harald Anlauf via Fortran a écrit : >> This LGTM. It also fixes a regression introduced with r9-3030 :-) >> If you think that this set (1-3) is backportable, feel free to do so. >> > Yes, 2 and 3 are worth backporting, I will see how dependent they are on 1. > I'm going to backport 1-3 as you suggested, it's so much easier.