Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Apr 22, 2011, Jason Merrill ja...@redhat.com wrote: On 04/22/2011 02:13 AM, Mike Stump wrote: http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html has the details of why the code was put in. Right. At the time, we were sorting the goto queue based on pointer values, which caused the problem. We no longer do that, so we shouldn't need this workaround (deferring creation of case label labels) anymore. What do you think, Alex? Yeah, this change looks safe now. -- Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist Red Hat Brazil Compiler Engineer
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Fri, Apr 22, 2011 at 12:59:21AM -0400, Jason Merrill wrote: On 04/21/2011 10:55 PM, Nathan Froyd wrote: On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote: Hunh. How does that work? They fill in CASE_LABEL later? Can that be changed? Yeah, tree-eh.c:lower_try_finally_switch. I don't know how easy it is to fix; it certainly looks non-trivial. Well, I tried adjusting it and regression testing seems fine so far. I can't think what the comment would be talking about with pointers not providing a stable order; I don't see anything that would rely on that. Based on discussion downthread, I plan to commit something like your patch (I think `label' is unused after this, so requires trivial changes) on your behalf tomorrow, unless you beat me to it or unless somebody yells. I'd rather have this not mixed up with the rest of the build_case_label changes. -Nathan
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Apr 21, 2011, at 9:59 PM, Jason Merrill wrote: On 04/21/2011 10:55 PM, Nathan Froyd wrote: On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote: Hunh. How does that work? They fill in CASE_LABEL later? Can that be changed? Yeah, tree-eh.c:lower_try_finally_switch. I don't know how easy it is to fix; it certainly looks non-trivial. Well, I tried adjusting it and regression testing seems fine so far. Unsurprising... It will never fail during testsuite run, and won't always fail during a bootstrap. I can't think what the comment would be talking about with pointers not providing a stable order; I don't see anything that would rely on that. http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html has the details of why the code was put in. Essentially, the Ada boostrap on x86 linux. What's worse is, at the time, it would only occasionally fail, so, a bootstrap that works won't prove anything.
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Fri, Apr 22, 2011 at 8:13 AM, Mike Stump mikest...@comcast.net wrote: On Apr 21, 2011, at 9:59 PM, Jason Merrill wrote: On 04/21/2011 10:55 PM, Nathan Froyd wrote: On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote: Hunh. How does that work? They fill in CASE_LABEL later? Can that be changed? Yeah, tree-eh.c:lower_try_finally_switch. I don't know how easy it is to fix; it certainly looks non-trivial. Well, I tried adjusting it and regression testing seems fine so far. Unsurprising... It will never fail during testsuite run, and won't always fail during a bootstrap. I can't think what the comment would be talking about with pointers not providing a stable order; I don't see anything that would rely on that. http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html has the details of why the code was put in. Essentially, the Ada boostrap on x86 linux. What's worse is, at the time, it would only occasionally fail, so, a bootstrap that works won't prove anything. Well, unless we are not walking a pointer-based hashtable I don't see how this matters here. To Nathan: yes, UNKNOWN_LOCATION would be correct. Whoever then sets the label should adjust it accordingly. Richard.
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Fri, Apr 22, 2011 at 11:12:01AM +0200, Richard Guenther wrote: On Fri, Apr 22, 2011 at 8:13 AM, Mike Stump mikest...@comcast.net wrote: Unsurprising... It will never fail during testsuite run, and won't always fail during a bootstrap. I can't think what the comment would be talking about with pointers not providing a stable order; I don't see anything that would rely on that. http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html has the details of why the code was put in. Essentially, the Ada boostrap on x86 linux. What's worse is, at the time, it would only occasionally fail, so, a bootstrap that works won't prove anything. Well, unless we are not walking a pointer-based hashtable I don't see how this matters here. I can't see the pointer traversal, either--unless there's some subtlety in how things are added to the goto_queue. I'm going to leave the code alone for the moment. To Nathan: yes, UNKNOWN_LOCATION would be correct. Whoever then sets the label should adjust it accordingly. Will commit with that change in build_case_label, then. I will leave the location-setting to a separate commit, if any. -Nathan
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On 04/22/2011 02:13 AM, Mike Stump wrote: http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html has the details of why the code was put in. Right. At the time, we were sorting the goto queue based on pointer values, which caused the problem. We no longer do that, so we shouldn't need this workaround (deferring creation of case label labels) anymore. What do you think, Alex? Jason
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Apr 22, 2011, at 8:55 AM, Jason Merrill wrote: On 04/22/2011 02:13 AM, Mike Stump wrote: http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html has the details of why the code was put in. Right. At the time, we were sorting the goto queue based on pointer values, which caused the problem. We no longer do that, Excellent. That was the only concern I knew of.
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Thu, 14 Apr 2011, Nathan Froyd wrote: On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote: On 03/24/2011 09:15 AM, Nathan Froyd wrote: + tree t = make_node (CASE_LABEL_EXPR); + + TREE_TYPE (t) = void_type_node; + SET_EXPR_LOCATION (t, input_location); As jsm and richi said, using input_location like this is a regression. Can we use DECL_SOURCE_LOCATION (label_decl) instead? Sure. Joseph, Richi, are you happy with that change? It would fix the C/C++ regression, as c_add_case_label does: /* Create the LABEL_DECL itself. */ label = create_artificial_label (loc); ... /* Add a CASE_LABEL to the statement-tree. */ case_label = add_stmt (build_case_label (loc, low_value, high_value, label)); so the DECL_SOURCE_LOCATION would be the same as the location_t we were passing in anyway. For the other languages, I think it would be neutral or an improvement (they all use input_location or UNKNOWN_LOCATION for the CASE_LABEL anyway). Seems fine with me. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote: On 03/24/2011 09:15 AM, Nathan Froyd wrote: + tree t = make_node (CASE_LABEL_EXPR); + + TREE_TYPE (t) = void_type_node; + SET_EXPR_LOCATION (t, input_location); As jsm and richi said, using input_location like this is a regression. Can we use DECL_SOURCE_LOCATION (label_decl) instead? I went off and tried that; some callers provide a NULL label_decl. What's the right thing to do in that case--use UNKNOWN_LOCATION or input_location? I'm leaning towards the former. -Nathan
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On 04/21/2011 08:50 PM, Nathan Froyd wrote: On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote: As jsm and richi said, using input_location like this is a regression. Can we use DECL_SOURCE_LOCATION (label_decl) instead? I went off and tried that; some callers provide a NULL label_decl. Hunh. How does that work? They fill in CASE_LABEL later? Can that be changed? Jason
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote: On 04/21/2011 08:50 PM, Nathan Froyd wrote: On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote: As jsm and richi said, using input_location like this is a regression. Can we use DECL_SOURCE_LOCATION (label_decl) instead? I went off and tried that; some callers provide a NULL label_decl. Hunh. How does that work? They fill in CASE_LABEL later? Can that be changed? Yeah, tree-eh.c:lower_try_finally_switch. I don't know how easy it is to fix; it certainly looks non-trivial. -Nathan
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote: On 03/24/2011 09:15 AM, Nathan Froyd wrote: + tree t = make_node (CASE_LABEL_EXPR); + + TREE_TYPE (t) = void_type_node; + SET_EXPR_LOCATION (t, input_location); As jsm and richi said, using input_location like this is a regression. Can we use DECL_SOURCE_LOCATION (label_decl) instead? Sure. Joseph, Richi, are you happy with that change? It would fix the C/C++ regression, as c_add_case_label does: /* Create the LABEL_DECL itself. */ label = create_artificial_label (loc); ... /* Add a CASE_LABEL to the statement-tree. */ case_label = add_stmt (build_case_label (loc, low_value, high_value, label)); so the DECL_SOURCE_LOCATION would be the same as the location_t we were passing in anyway. For the other languages, I think it would be neutral or an improvement (they all use input_location or UNKNOWN_LOCATION for the CASE_LABEL anyway). [PATCH 11/18] mark EXPR_PACK_EXPANSION as typed only http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00563.html It looks like you need to add EXPR_PACK_EXPANSION cases to value_dependent_expression_p and cp_tree_equal. Maybe split out the code from write_expression that overrides TREE_OPERAND_LENGTH in some cases and use that new function instead of TREE_OPERAND_LENGTH in these places. Thanks for catching this, will do. -Nathan
Re: [PATCH PING] c++-specific bits of tree-slimming patches
On 03/24/2011 09:15 AM, Nathan Froyd wrote: The C++-specific bits of these patches: [PATCH 02/18] enforce TREE_CHAIN and TREE_TYPE accesses http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00557.html OK. [PATCH 07/18] generalize build_case_label to the rest of the compiler http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00557.html + tree t = make_node (CASE_LABEL_EXPR); + + TREE_TYPE (t) = void_type_node; + SET_EXPR_LOCATION (t, input_location); As jsm and richi said, using input_location like this is a regression. Can we use DECL_SOURCE_LOCATION (label_decl) instead? [PATCH 08/18] convert cp *FOR_STMTs to use private scope fields http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00553.html [PATCH 09/18] convert cp IF_STMTs to use private scope fields http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00562.html [PATCH 10/18] convert cp SWITCH_STMTs to use private scope fields http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00552.html OK. [PATCH 11/18] mark EXPR_PACK_EXPANSION as typed only http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00563.html It looks like you need to add EXPR_PACK_EXPANSION cases to value_dependent_expression_p and cp_tree_equal. Maybe split out the code from write_expression that overrides TREE_OPERAND_LENGTH in some cases and use that new function instead of TREE_OPERAND_LENGTH in these places. [PATCH 17/18] introduce block_chainon and use BLOCK_CHAIN more http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00566.html OK. Alternatively, could we have a GWP or similar rule on Tom's suggestion: http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00620.html that patches propagating middle-end changes to front-ends are obvious/preapproved, perhaps after a 24-48 hour waiting period for comments? That would cover 02 and 07 above (possibly 17 as well); 02 is blocking some of the already-approved middle-end changes later in the series. That makes sense to me, but I'd give it a week. Jason
