| Message ID | 20230323203507.2960052-1-jason@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] | expand |
On Thu, Mar 23, 2023 at 04:35:07PM -0400, Jason Merrill wrote: > Tested x86_64-pc-linux-gnu. Jakub, does this make sense to you? Do we have a > way of testing for compile-hog regressions? > > -- 8< -- > > The patch for PR91415 fixed -Wsequence-point to treat shifts and ARRAY_REF > as sequenced in C++17, and COMPONENT_REF as well. But this is unnecessary > for COMPONENT_REF, since the RHS is just a FIELD_DECL with no actual > evaluation, and in this testcase handling COMPONENT_REF as sequenced blows > up fast in a deep inheritance tree. > > PR c++/107163 > > gcc/c-family/ChangeLog: > > * c-common.cc (verify_tree): Don't use sequenced handling > for COMPONENT_REF. When we touch this for COMPONENT_REF, shouldn't we then handle it as unary given that the second operand is FIELD_DECL and third/fourth will likely be NULL and even if not, aren't user expressions that should be inspected? So, instead of doing this do: case COMPONENT_REF: x = TREE_OPERAND (x, 0); writer = 0; goto restart; ? As for compile-hog, depends on how long it will take it to compile before fix/after fix. If before fix can be above the normal timeout on reasonably fast matchines and after fix can take a few seconds, great, if after fix would take longer but still not horribly long, one way to do it is guard the test with run_expensive_tests effective target. Or another way is have the test smaller in complexity normally and // { dg-additional-options "-DEXPENSIVE" { target run_expensive_tests } } and #ifdef EXPENSIVE make it more complex. Jakub
On 3/23/23 17:03, Jakub Jelinek wrote: > On Thu, Mar 23, 2023 at 04:35:07PM -0400, Jason Merrill wrote: >> Tested x86_64-pc-linux-gnu. Jakub, does this make sense to you? Do we have a >> way of testing for compile-hog regressions? >> >> -- 8< -- >> >> The patch for PR91415 fixed -Wsequence-point to treat shifts and ARRAY_REF >> as sequenced in C++17, and COMPONENT_REF as well. But this is unnecessary >> for COMPONENT_REF, since the RHS is just a FIELD_DECL with no actual >> evaluation, and in this testcase handling COMPONENT_REF as sequenced blows >> up fast in a deep inheritance tree. >> >> PR c++/107163 >> >> gcc/c-family/ChangeLog: >> >> * c-common.cc (verify_tree): Don't use sequenced handling >> for COMPONENT_REF. > > When we touch this for COMPONENT_REF, shouldn't we then handle it as > unary given that the second operand is FIELD_DECL and third/fourth > will likely be NULL and even if not, aren't user expressions that should be > inspected? > So, instead of doing this do: > case COMPONENT_REF: > x = TREE_OPERAND (x, 0); > writer = 0; > goto restart; > ? Is clearing 'writer' what we want, since an access to COMPONENT_REF is an access to (a subobject of) its op0? > As for compile-hog, depends on how long it will take it to compile before > fix/after fix. If before fix can be above the normal timeout on reasonably > fast matchines and after fix can take a few seconds, great Currently with the fix it takes <1s while gcc12 takes ~80s. > if after fix > would take longer but still not horribly long, one way to do it is > guard the test with run_expensive_tests effective target. Or another way > is have the test smaller in complexity normally and > // { dg-additional-options "-DEXPENSIVE" { target run_expensive_tests } } > and #ifdef EXPENSIVE make it more complex. Curiously, making the recursion much deeper doesn't work for that; I guess at some point the -Wsequence-point code decides the expression is too complex and gives up? But repeating the assignment brings it up over the timeout. How about this?
On Fri, Mar 24, 2023 at 06:11:44PM -0400, Jason Merrill wrote: > > When we touch this for COMPONENT_REF, shouldn't we then handle it as > > unary given that the second operand is FIELD_DECL and third/fourth > > will likely be NULL and even if not, aren't user expressions that should be > > inspected? > > So, instead of doing this do: > > case COMPONENT_REF: > > x = TREE_OPERAND (x, 0); > > writer = 0; > > goto restart; > > ? > > Is clearing 'writer' what we want, since an access to COMPONENT_REF is an > access to (a subobject of) its op0? I've just mindlessly copied the unary op case. writer is set for pre/post increments and lhs of MODIFY_EXPR, and it is true that VIEW_CONVERT_EXPR doesn't clear it, but e.g. ARRAY_REF clears it for all operands. > Currently with the fix it takes <1s while gcc12 takes ~80s. Perfect. > PR c++/107163 > > gcc/c-family/ChangeLog: > > * c-common.cc (verify_tree): Don't use sequenced handling > for COMPONENT_REF. > > gcc/testsuite/ChangeLog: > > * g++.dg/template/recurse5.C: New test. LGTM, thanks. Maybe the testcase would be better as warn/Wsequence-point-5.C, dunno. Jakub
On 3/24/23 18:25, Jakub Jelinek wrote: > On Fri, Mar 24, 2023 at 06:11:44PM -0400, Jason Merrill wrote: >>> When we touch this for COMPONENT_REF, shouldn't we then handle it as >>> unary given that the second operand is FIELD_DECL and third/fourth >>> will likely be NULL and even if not, aren't user expressions that should be >>> inspected? >>> So, instead of doing this do: >>> case COMPONENT_REF: >>> x = TREE_OPERAND (x, 0); >>> writer = 0; >>> goto restart; >>> ? >> >> Is clearing 'writer' what we want, since an access to COMPONENT_REF is an >> access to (a subobject of) its op0? > > I've just mindlessly copied the unary op case. > writer is set for pre/post increments and lhs of MODIFY_EXPR, and it is > true that VIEW_CONVERT_EXPR doesn't clear it, but e.g. ARRAY_REF clears it > for all operands. For whatever reason leaving writer set led to lots of false positives, so I've gone with your suggestion. >> Currently with the fix it takes <1s while gcc12 takes ~80s. > > Perfect. > >> PR c++/107163 >> >> gcc/c-family/ChangeLog: >> >> * c-common.cc (verify_tree): Don't use sequenced handling >> for COMPONENT_REF. >> >> gcc/testsuite/ChangeLog: >> >> * g++.dg/template/recurse5.C: New test. > > LGTM, thanks. Maybe the testcase would be better as > warn/Wsequence-point-5.C, dunno. Done. Jason
diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc index bfb950e56db..a803cf94c68 100644 --- a/gcc/c-family/c-common.cc +++ b/gcc/c-family/c-common.cc @@ -2154,7 +2154,6 @@ verify_tree (tree x, struct tlist **pbefore_sp, struct tlist **pno_sp, case LSHIFT_EXPR: case RSHIFT_EXPR: - case COMPONENT_REF: case ARRAY_REF: if (cxx_dialect >= cxx17) goto sequenced_binary;