[PATCH V2 0/4] Unify C and C++ handling of loops and switches
This is a revised version of the patch set originally posted last November: https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534142.html In addition to generally updating and rebasing the patches to reflect other changes on mainline in the meantime, for this version I have switched to using the C lowering strategy (directly to goto form) rather than the C++ one (to LOOP_EXPR) because of regressions in the C optimization tests. Besides the ones previously noted in the original patch submission, there were a bunch of new ones since November. Some of them were trivial to fix (e.g., flipping branch probabilities to reflect the different sense of the loop exit condition in the C++-style output), but I wasn't making much progress on others and eventually decided to pursue the "plan B" of using the C-style output everywhere, as discussed here: https://gcc.gnu.org/pipermail/gcc-patches/2019-December/536536.html The only regression I ran into with this was a bootstrap failure building the Fortran front end from a new -Wmaybe-uninitialized error. This might be a false positive but part 3 of the new series works around it by adding an assertion to give g++ a hint. Unfortunately I had no luck in trying to reduce this to a standalone test case, but I did observe that the failure went away when I compiled that file with debugging enabled. :-S I could file a PR to look into this further if the workaround is good enough for now. -Sandra Sandra Loosemore (4): Move loop and switch tree data structures from cp/ to c-family/. Use C-style loop lowering instead of C++-style. Work around bootstrap failure in Fortran front end. Change C front end to emit structured loop and switch tree nodes. gcc/c-family/c-common.c | 24 ++ gcc/c-family/c-common.def | 24 ++ gcc/c-family/c-common.h | 53 +++- gcc/c-family/c-dump.c | 38 +++ gcc/c-family/c-gimplify.c | 422 gcc/c-family/c-pretty-print.c | 92 ++- gcc/c/c-decl.c | 18 +- gcc/c/c-lang.h | 3 +- gcc/c/c-objc-common.h | 2 + gcc/c/c-parser.c| 125 +- gcc/c/c-tree.h | 21 +- gcc/c/c-typeck.c| 227 ++--- gcc/cp/cp-gimplify.c| 469 +++- gcc/cp/cp-objcp-common.c| 13 +- gcc/cp/cp-tree.def | 23 -- gcc/cp/cp-tree.h| 40 --- gcc/cp/cxx-pretty-print.c | 78 -- gcc/cp/dump.c | 31 --- gcc/doc/generic.texi| 56 +++-- gcc/fortran/interface.c | 4 + gcc/objc/ChangeLog | 5 + gcc/objc/objc-act.c | 6 +- gcc/testsuite/gcc.dg/gomp/block-7.c | 12 +- 23 files changed, 938 insertions(+), 848 deletions(-) -- 2.8.1
[PING] [PATCH V2 0/4] Unify C and C++ handling of loops and switches
Ping! Only the fix for the Fortran bootstrap failure in part 3 has been reviewed. -Sandra https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551927.html On 8/13/20 10:34 AM, Sandra Loosemore wrote: This is a revised version of the patch set originally posted last November: https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534142.html In addition to generally updating and rebasing the patches to reflect other changes on mainline in the meantime, for this version I have switched to using the C lowering strategy (directly to goto form) rather than the C++ one (to LOOP_EXPR) because of regressions in the C optimization tests. Besides the ones previously noted in the original patch submission, there were a bunch of new ones since November. Some of them were trivial to fix (e.g., flipping branch probabilities to reflect the different sense of the loop exit condition in the C++-style output), but I wasn't making much progress on others and eventually decided to pursue the "plan B" of using the C-style output everywhere, as discussed here: https://gcc.gnu.org/pipermail/gcc-patches/2019-December/536536.html The only regression I ran into with this was a bootstrap failure building the Fortran front end from a new -Wmaybe-uninitialized error. This might be a false positive but part 3 of the new series works around it by adding an assertion to give g++ a hint. Unfortunately I had no luck in trying to reduce this to a standalone test case, but I did observe that the failure went away when I compiled that file with debugging enabled. :-S I could file a PR to look into this further if the workaround is good enough for now. -Sandra Sandra Loosemore (4): Move loop and switch tree data structures from cp/ to c-family/. Use C-style loop lowering instead of C++-style. Work around bootstrap failure in Fortran front end. Change C front end to emit structured loop and switch tree nodes. gcc/c-family/c-common.c | 24 ++ gcc/c-family/c-common.def | 24 ++ gcc/c-family/c-common.h | 53 +++- gcc/c-family/c-dump.c | 38 +++ gcc/c-family/c-gimplify.c | 422 gcc/c-family/c-pretty-print.c | 92 ++- gcc/c/c-decl.c | 18 +- gcc/c/c-lang.h | 3 +- gcc/c/c-objc-common.h | 2 + gcc/c/c-parser.c| 125 +- gcc/c/c-tree.h | 21 +- gcc/c/c-typeck.c| 227 ++--- gcc/cp/cp-gimplify.c| 469 +++- gcc/cp/cp-objcp-common.c| 13 +- gcc/cp/cp-tree.def | 23 -- gcc/cp/cp-tree.h| 40 --- gcc/cp/cxx-pretty-print.c | 78 -- gcc/cp/dump.c | 31 --- gcc/doc/generic.texi| 56 +++-- gcc/fortran/interface.c | 4 + gcc/objc/ChangeLog | 5 + gcc/objc/objc-act.c | 6 +- gcc/testsuite/gcc.dg/gomp/block-7.c | 12 +- 23 files changed, 938 insertions(+), 848 deletions(-)
Re: [PATCH V2 0/4] Unify C and C++ handling of loops and switches
On 8/13/20 12:34 PM, Sandra Loosemore wrote: This is a revised version of the patch set originally posted last November: https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534142.html In addition to generally updating and rebasing the patches to reflect other changes on mainline in the meantime, for this version I have switched to using the C lowering strategy (directly to goto form) rather than the C++ one (to LOOP_EXPR) because of regressions in the C optimization tests. Besides the ones previously noted in the original patch submission, there were a bunch of new ones since November. Some of them were trivial to fix (e.g., flipping branch probabilities to reflect the different sense of the loop exit condition in the C++-style output), but I wasn't making much progress on others and eventually decided to pursue the "plan B" of using the C-style output everywhere, as discussed here: https://gcc.gnu.org/pipermail/gcc-patches/2019-December/536536.html The only regression I ran into with this was a bootstrap failure building the Fortran front end from a new -Wmaybe-uninitialized error. This might be a false positive but part 3 of the new series works around it by adding an assertion to give g++ a hint. Unfortunately I had no luck in trying to reduce this to a standalone test case, but I did observe that the failure went away when I compiled that file with debugging enabled. :-S I could file a PR to look into this further if the workaround is good enough for now. My impression from Jeff's analysis in January and David's in March was that many of the testsuite changes were from the C++ approach actually providing better results, so the reversal here surprises me. Can you talk more about the regressions you're seeing? Jason
Re: [PATCH V2 0/4] Unify C and C++ handling of loops and switches
On 9/9/20 3:13 PM, Jason Merrill wrote: My impression from Jeff's analysis in January and David's in March was that many of the testsuite changes were from the C++ approach actually providing better results, so the reversal here surprises me. Can you talk more about the regressions you're seeing? I spent most of my time earlier in the summer looking at the 3 regressions I originally saw last fall. Unfortunately I could get no traction at all on the 2 FSA-related ones. :-( For the gcc.dg/tree-ssa/ssa-dce-3.c regression, I tracked it down to some code in the cddce1 pass being sensitive to the ordering of basic blocks in the input code; I filed PR96487 for that. I was also having a bunch of problems with -Wimplicit-fallthrough failures triggered by the C++ front end loop expansion sometimes flipping the sense of the end test conditional (through the use of fold_build3_loc to build it). It seemed to me that the code that handles COND_EXPRs for those warnings is sensitive to the order of blocks as well, and has asymmetric assumptions that the code for the "if" branch is emitted inline before the "else" branch. I tried some experiments with generalizing it to recognize the branches in either order, but that did not fix the regressions, so maybe the problem was somewhere else entirely, or a combination of two different bugs. :-( Anyway it seemed to me that the patches would not be accepted if I resubmitted them in a form that still caused regressions, and switching back to the C way of expanding to GOTO form directly instead of via LOOP_EXPR did that. -Sandra
Re: [PATCH V2 0/4] Unify C and C++ handling of loops and switches
On Wed, 2020-09-09 at 17:13 -0400, Jason Merrill wrote: > On 8/13/20 12:34 PM, Sandra Loosemore wrote: > > This is a revised version of the patch set originally posted > > last November: > > > > https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534142.html > > > > In addition to generally updating and rebasing the patches to > > reflect > > other changes on mainline in the meantime, for this version I have > > switched to using the C lowering strategy (directly to goto form) > > rather than the C++ one (to LOOP_EXPR) because of regressions in > > the C > > optimization tests. Besides the ones previously noted in the > > original > > patch submission, there were a bunch of new ones since > > November. Some > > of them were trivial to fix (e.g., flipping branch probabilities to > > reflect the different sense of the loop exit condition in the > > C++-style output), but I wasn't making much progress on others and > > eventually decided to pursue the "plan B" of using the C-style > > output > > everywhere, as discussed here: > > > > https://gcc.gnu.org/pipermail/gcc-patches/2019-December/536536.html > > > > The only regression I ran into with this was a bootstrap failure > > building the Fortran front end from a new -Wmaybe-uninitialized > > error. > > This might be a false positive but part 3 of the new series works > > around it by adding an assertion to give g++ a hint. Unfortunately > > I > > had no luck in trying to reduce this to a standalone test case, but > > I > > did observe that the failure went away when I compiled that file > > with > > debugging enabled. :-S I could file a PR to look into this > > further if > > the workaround is good enough for now. > > My impression from Jeff's analysis in January and David's in March For reference, these seem to have been: https://gcc.gnu.org/pipermail/gcc-patches/2020-January/539207.html and: https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542018.html respectively. In the latter I said: > I think that what's happened is that your patch improves the location > information for the gimple stmts, and this uncovers a bug in how I > wrote the test. which is presumably what Jason's getting at. BTW, in terms of analyzer issues, my big rewrite of analyzer state- tracking landed in master a month ago, on 2020-08-13 as 808f4dfeb3a95f50f15e71148e5c1067f90a126d, and changed the behavior of gcc.dg/analyzer/explode-2.c, which was one of the ones an earlier version of Sandra's patch also affected. Sandra: was the most recent patch you posted tested against a relatively recent source tree? (i.e. since that 2020-08-13 patch?). That said, I don't want the analyzer to stand in the way of improvements to the frontends, so let me know if I can help with any analyzer testsuite regressions. Thanks Dave > was > that many of the testsuite changes were from the C++ approach > actually > providing better results, so the reversal here surprises me. Can > you > talk more about the regressions you're seeing?
Re: [PATCH V2 0/4] Unify C and C++ handling of loops and switches
On 9/10/20 7:36 AM, David Malcolm wrote: BTW, in terms of analyzer issues, my big rewrite of analyzer state- tracking landed in master a month ago, on 2020-08-13 as 808f4dfeb3a95f50f15e71148e5c1067f90a126d, and changed the behavior of gcc.dg/analyzer/explode-2.c, which was one of the ones an earlier version of Sandra's patch also affected. In case I failed to make this explicit previously: my 2020-08-13 patch did not trigger the analyzer regressions in C that the version from last fall did, because it retains the existing form of C lowering for loops. Sandra: was the most recent patch you posted tested against a relatively recent source tree? (i.e. since that 2020-08-13 patch?). That said, I don't want the analyzer to stand in the way of improvements to the frontends, so let me know if I can help with any analyzer testsuite regressions. I did not prepare a rebased patch set against mainline head, but I did just rebase, rebuild, and retest these patches earlier this week in conjunction with the OpenACC kernels loop annotation patches that depend on them, that I just posted yesterday. I didn't see any analyzer regressions in those test results either. -Sandra
Re: [PATCH V2 0/4] Unify C and C++ handling of loops and switches
On 9/9/20 8:20 PM, Sandra Loosemore wrote: On 9/9/20 3:13 PM, Jason Merrill wrote: My impression from Jeff's analysis in January and David's in March was that many of the testsuite changes were from the C++ approach actually providing better results, so the reversal here surprises me. Can you talk more about the regressions you're seeing? I spent most of my time earlier in the summer looking at the 3 regressions I originally saw last fall. Unfortunately I could get no traction at all on the 2 FSA-related ones. :-( For the gcc.dg/tree-ssa/ssa-dce-3.c regression, I tracked it down to some code in the cddce1 pass being sensitive to the ordering of basic blocks in the input code; I filed PR96487 for that. I was also having a bunch of problems with -Wimplicit-fallthrough failures triggered by the C++ front end loop expansion sometimes flipping the sense of the end test conditional (through the use of fold_build3_loc to build it). It seemed to me that the code that handles COND_EXPRs for those warnings is sensitive to the order of blocks as well, and has asymmetric assumptions that the code for the "if" branch is emitted inline before the "else" branch. I tried some experiments with generalizing it to recognize the branches in either order, but that did not fix the regressions, so maybe the problem was somewhere else entirely, or a combination of two different bugs. :-( Anyway it seemed to me that the patches would not be accepted if I resubmitted them in a form that still caused regressions, and switching back to the C way of expanding to GOTO form directly instead of via LOOP_EXPR did that. We discussed this in a team meeting the other day, and agreed that it's probably simpler to switch back to gotos for C++ than fix up all the optimizers. And that there probably isn't much benefit to the middle-end to retain the higher level representation longer. Though the patch needed to the Fortran compiler sounds like a regression from this change. Has someone looked at where that's coming from? Have you done any benchmark comparison for C++ tests with this change? If not, please plan to monitor the lnt.opensuse.org benchmark results for impact after the change is committed. The C++ changes are OK. A C maintainer will need to sign off on the changes there. Jason
Re: [PATCH V2 0/4] Unify C and C++ handling of loops and switches
On Thu, 17 Sep 2020, Jason Merrill via Gcc-patches wrote: > The C++ changes are OK. A C maintainer will need to sign off on the changes > there. The C front-end changes are OK. Note: for a long time there used to be actual (undesired) semantic differences between the C and C++ loop handling, but that was fixed with a testcase added (see bug 44715), so I'm not expecting this patch to change C semantics at all. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH V2 0/4] Unify C and C++ handling of loops and switches
On 9/17/20 8:32 AM, Jason Merrill wrote: > On 9/9/20 8:20 PM, Sandra Loosemore wrote: >> On 9/9/20 3:13 PM, Jason Merrill wrote: >>> >>> My impression from Jeff's analysis in January and David's in March >>> was that many of the testsuite changes were from the C++ approach >>> actually providing better results, so the reversal here surprises >>> me. Can you talk more about the regressions you're seeing? >> >> I spent most of my time earlier in the summer looking at the 3 >> regressions I originally saw last fall. Unfortunately I could get no >> traction at all on the 2 FSA-related ones. :-( For the >> gcc.dg/tree-ssa/ssa-dce-3.c regression, I tracked it down to some >> code in the cddce1 pass being sensitive to the ordering of basic >> blocks in the input code; I filed PR96487 for that. >> >> I was also having a bunch of problems with -Wimplicit-fallthrough >> failures triggered by the C++ front end loop expansion sometimes >> flipping the sense of the end test conditional (through the use of >> fold_build3_loc to build it). It seemed to me that the code that >> handles COND_EXPRs for those warnings is sensitive to the order of >> blocks as well, and has asymmetric assumptions that the code for the >> "if" branch is emitted inline before the "else" branch. I tried some >> experiments with generalizing it to recognize the branches in either >> order, but that did not fix the regressions, so maybe the problem was >> somewhere else entirely, or a combination of two different bugs. :-( >> >> Anyway it seemed to me that the patches would not be accepted if I >> resubmitted them in a form that still caused regressions, and >> switching back to the C way of expanding to GOTO form directly >> instead of via LOOP_EXPR did that. > > We discussed this in a team meeting the other day, and agreed that > it's probably simpler to switch back to gotos for C++ than fix up all > the optimizers. And that there probably isn't much benefit to the > middle-end to retain the higher level representation longer. Right. I would expect all of Richi's work through the years in loop discovery and canonicalization to eliminate most, if not all, of the benefit of LOOP_EXPR. Ultimately I think we just pick one style and use it in both places. So I'm not going to object to this patch. > The C++ changes are OK. A C maintainer will need to sign off on the > changes there. And Joseph just signed off on the C bits. > > Jason > pEpkey.asc Description: application/pgp-keys
Re: [PATCH V2 0/4] Unify C and C++ handling of loops and switches
On 9/17/20 8:32 AM, Jason Merrill wrote: We discussed this in a team meeting the other day, and agreed that it's probably simpler to switch back to gotos for C++ than fix up all the optimizers. And that there probably isn't much benefit to the middle-end to retain the higher level representation longer. When I looked into the history of the C++ change to use LOOP_EXPR, it seemed the primary motivation was to allow the constexpr evaluator to recognize loops and the optimization benefits were only lightly tested. But now the constexpr evaluator doesn't need LOOP_EXPR, and meanwhile I think the loop infrastructure and loop optimization test cases have been tuned for the C-style output. Though the patch needed to the Fortran compiler sounds like a regression from this change. Has someone looked at where that's coming from? I tried, but wasn't able to come up with a self-contained test case small enough that I could actually analyze what it was doing, but that still triggered the warning. In fact I am still unsure whether it is a bug that we weren't diagnosing that warning before, rather than that it showed up now. :-S As a programmer looking at that code, I thought the warning was justifiable, at least. Have you done any benchmark comparison for C++ tests with this change? If not, please plan to monitor the lnt.opensuse.org benchmark results for impact after the change is committed. I haven't done any benchmarking myself, but yes, I will keep an eye on those benchmark results. The C++ changes are OK. A C maintainer will need to sign off on the changes there. Thanks for the review! -Sandra
Re: [PING^2] [PATCH V2 0/4] Unify C and C++ handling of loops and switches
Ping again on this patch series: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551927.html These patches just missed making it into GCC 10 last year -- although there seemed to be agreement in principle, they needed a bit more work to resolve test regressions. Now that we are heading into fall again, I am worried that they may miss GCC 11 as well if they need further re-working but I don't get feedback until very late in the release cycle, or any feedback at all. :-( I also have a set of OpenACC patches for identifying loops in kernels regions that depend on these; I'll be posting those shortly and I hope to get those into GCC 11 as well. -Sandra On 8/13/20 10:34 AM, Sandra Loosemore wrote: This is a revised version of the patch set originally posted last November: https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534142.html In addition to generally updating and rebasing the patches to reflect other changes on mainline in the meantime, for this version I have switched to using the C lowering strategy (directly to goto form) rather than the C++ one (to LOOP_EXPR) because of regressions in the C optimization tests. Besides the ones previously noted in the original patch submission, there were a bunch of new ones since November. Some of them were trivial to fix (e.g., flipping branch probabilities to reflect the different sense of the loop exit condition in the C++-style output), but I wasn't making much progress on others and eventually decided to pursue the "plan B" of using the C-style output everywhere, as discussed here: https://gcc.gnu.org/pipermail/gcc-patches/2019-December/536536.html The only regression I ran into with this was a bootstrap failure building the Fortran front end from a new -Wmaybe-uninitialized error. This might be a false positive but part 3 of the new series works around it by adding an assertion to give g++ a hint. Unfortunately I had no luck in trying to reduce this to a standalone test case, but I did observe that the failure went away when I compiled that file with debugging enabled. :-S I could file a PR to look into this further if the workaround is good enough for now. -Sandra Sandra Loosemore (4): Move loop and switch tree data structures from cp/ to c-family/. Use C-style loop lowering instead of C++-style. Work around bootstrap failure in Fortran front end. Change C front end to emit structured loop and switch tree nodes. gcc/c-family/c-common.c | 24 ++ gcc/c-family/c-common.def | 24 ++ gcc/c-family/c-common.h | 53 +++- gcc/c-family/c-dump.c | 38 +++ gcc/c-family/c-gimplify.c | 422 gcc/c-family/c-pretty-print.c | 92 ++- gcc/c/c-decl.c | 18 +- gcc/c/c-lang.h | 3 +- gcc/c/c-objc-common.h | 2 + gcc/c/c-parser.c | 125 +- gcc/c/c-tree.h | 21 +- gcc/c/c-typeck.c | 227 ++--- gcc/cp/cp-gimplify.c | 469 +++- gcc/cp/cp-objcp-common.c | 13 +- gcc/cp/cp-tree.def | 23 -- gcc/cp/cp-tree.h | 40 --- gcc/cp/cxx-pretty-print.c | 78 -- gcc/cp/dump.c | 31 --- gcc/doc/generic.texi | 56 +++-- gcc/fortran/interface.c | 4 + gcc/objc/ChangeLog | 5 + gcc/objc/objc-act.c | 6 +- gcc/testsuite/gcc.dg/gomp/block-7.c | 12 +- 23 files changed, 938 insertions(+), 848 deletions(-)
