Re: [PATCH 1/4] force decls to be allocated through build_decl to initialize them
On Thu, Jul 15, 2021 at 10:01:01AM +0200, Richard Biener wrote: > On Thu, Jul 15, 2021 at 4:24 AM Trevor Saunders wrote: > > > > On Wed, Jul 14, 2021 at 01:27:54PM +0200, Richard Biener wrote: > > > On Wed, Jul 14, 2021 at 10:20 AM Trevor Saunders > > > wrote: > > > > > > > > prior to this commit all calls to build_decl used input_location, even > > > > if > > > > temporarily until build_decl reset the location to something else that > > > > it was > > > > told was the proper location. To avoid using the global we need the > > > > caller to > > > > pass in the location it wants, however that's not possible with > > > > make_node since > > > > it makes other types of nodes. So we force all callers who wish to > > > > make a decl > > > > to go through build_decl which already takes a location argument. To > > > > avoid > > > > changing behavior this just explicitly passes in input_location to > > > > build_decl > > > > for callers of make_node that create a decl, however it would seem in > > > > many of > > > > these cases that the location of the decl being coppied might be a > > > > better > > > > location. > > > > > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > > > > > I think all eventually DECL_ARTIFICIAL decls should better use > > > UNKNOWN_LOCATION instead of input_location. > > > > You'd know if that might break something better than me, but that seems > > sensible in principal. That said, I would like to incrementally do one > > thing at a time, rather than change make_node to use unknown_location, > > and set the location to something else all at once, but I suppose I > > could first change some callers to be build_decl (unknown_location, ...) > > and then come back to changing make_node when there's fewer callers to > > reason about if that's preferable. > > Sure, we can defer changing make_node (I thought the patch catched all > but three callers ...). But it feels odd to introduce so many explicit > input_location uses for cases where it clearly doesn't matter (the > DECL_ARTIFICIAL), > so I'd prefer to "fix" those immediately, Fair enough, I think you just have more expereience and confidence it can't matter than I do yet, I'll work on fixing them immediately. Trev
Re: [PATCH 1/4] force decls to be allocated through build_decl to initialize them
On Thu, Jul 15, 2021 at 4:24 AM Trevor Saunders wrote: > > On Wed, Jul 14, 2021 at 01:27:54PM +0200, Richard Biener wrote: > > On Wed, Jul 14, 2021 at 10:20 AM Trevor Saunders > > wrote: > > > > > > prior to this commit all calls to build_decl used input_location, even if > > > temporarily until build_decl reset the location to something else that > > > it was > > > told was the proper location. To avoid using the global we need the > > > caller to > > > pass in the location it wants, however that's not possible with make_node > > > since > > > it makes other types of nodes. So we force all callers who wish to make > > > a decl > > > to go through build_decl which already takes a location argument. To > > > avoid > > > changing behavior this just explicitly passes in input_location to > > > build_decl > > > for callers of make_node that create a decl, however it would seem in > > > many of > > > these cases that the location of the decl being coppied might be a better > > > location. > > > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > > > I think all eventually DECL_ARTIFICIAL decls should better use > > UNKNOWN_LOCATION instead of input_location. > > You'd know if that might break something better than me, but that seems > sensible in principal. That said, I would like to incrementally do one > thing at a time, rather than change make_node to use unknown_location, > and set the location to something else all at once, but I suppose I > could first change some callers to be build_decl (unknown_location, ...) > and then come back to changing make_node when there's fewer callers to > reason about if that's preferable. Sure, we can defer changing make_node (I thought the patch catched all but three callers ...). But it feels odd to introduce so many explicit input_location uses for cases where it clearly doesn't matter (the DECL_ARTIFICIAL), so I'd prefer to "fix" those immediately, > > I'm not sure if I like the (transitional) extra arg to make_node, I suppose > > we could hide make_node by declaring it in tree-raw.h or so or by > > guarding the decl with NEED_MAKE_NODE. There's nothing inherently > > wrong with calling make_node. So what I mean with transitional is that > > with this change we should simply set the location to UNKNOWN_LOCATION > > (aka zero, which it already is), not input_location, in make_node. > > I sort of think it makes sense to move all the tree class specific bits > out of make_node to functions for that specific type of tree, but it is > mostly unrelated. One advantage of that is that it saves pointless > initialization in the module / lto streamer that gets over written with > the streamed values. However having used the argument to find all the > places that create decls, and having updated them, while the argument > and asserts do prevent leaving the location uninitialized by mistake, > I'd be fine with dropping that part and just updating all the make_node > callers to use build_decl. Yes, that's my thinking. Thanks, Richard. > > thanks > > Trev > > > > > Richard. > > > > > Trev > > > > > > gcc/ChangeLog: > > > > > > * cfgexpand.c (avoid_deep_ter_for_debug): Call build_decl not > > > make_node. > > > (expand_gimple_basic_block): Likewise. > > > * ipa-param-manipulation.c (ipa_param_adjustments::modify_call): > > > * Likewise. > > > (ipa_param_body_adjustments::reset_debug_stmts): Likewise. > > > * omp-simd-clone.c (ipa_simd_modify_stmt_ops): Likewise. > > > * stor-layout.c (start_bitfield_representative): Likewise. > > > * tree-inline.c (remap_ssa_name): Likewise. > > > (tree_function_versioning): Likewise. > > > * tree-into-ssa.c (rewrite_debug_stmt_uses): Likewise. > > > * tree-nested.c (lookup_field_for_decl): Likewise. > > > (get_chain_field): Likewise. > > > (create_field_for_decl): Likewise. > > > (get_nl_goto_field): Likewise. > > > (finalize_nesting_tree_1): Likewise. > > > * tree-ssa-ccp.c (optimize_atomic_bit_test_and): Likewise. > > > * tree-ssa-loop-ivopts.c (remove_unused_ivs): Likewise. > > > * tree-ssa-phiopt.c (spaceship_replacement): Likewise. > > > * tree-ssa-reassoc.c (make_new_ssa_for_def): Likewise. > > > * tree-ssa.c (insert_debug_temp_for_var_def): Likewise. > > > * tree-streamer-in.c (streamer_alloc_tree): Adjust. > > > * tree.c (make_node): Add argument to specify the caller. > > > (build_decl): Move initialization from make_node. > > > * tree.h (enum make_node_caller): new enum. > > > (make_node): Adjust prototype. > > > * varasm.c (make_debug_expr_from_rtl): call build_decl. > > > > > > gcc/cp/ChangeLog: > > > > > > * constraint.cc (build_type_constraint): Call build_decl not > > > make_node. > > > * cp-gimplify.c (cp_genericize_r): Likewise. > > > * parser.c
Re: [PATCH 1/4] force decls to be allocated through build_decl to initialize them
On Wed, Jul 14, 2021 at 01:27:54PM +0200, Richard Biener wrote: > On Wed, Jul 14, 2021 at 10:20 AM Trevor Saunders > wrote: > > > > prior to this commit all calls to build_decl used input_location, even if > > temporarily until build_decl reset the location to something else that it > > was > > told was the proper location. To avoid using the global we need the caller > > to > > pass in the location it wants, however that's not possible with make_node > > since > > it makes other types of nodes. So we force all callers who wish to make a > > decl > > to go through build_decl which already takes a location argument. To avoid > > changing behavior this just explicitly passes in input_location to > > build_decl > > for callers of make_node that create a decl, however it would seem in many > > of > > these cases that the location of the decl being coppied might be a better > > location. > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > I think all eventually DECL_ARTIFICIAL decls should better use > UNKNOWN_LOCATION instead of input_location. You'd know if that might break something better than me, but that seems sensible in principal. That said, I would like to incrementally do one thing at a time, rather than change make_node to use unknown_location, and set the location to something else all at once, but I suppose I could first change some callers to be build_decl (unknown_location, ...) and then come back to changing make_node when there's fewer callers to reason about if that's preferable. > I'm not sure if I like the (transitional) extra arg to make_node, I suppose > we could hide make_node by declaring it in tree-raw.h or so or by > guarding the decl with NEED_MAKE_NODE. There's nothing inherently > wrong with calling make_node. So what I mean with transitional is that > with this change we should simply set the location to UNKNOWN_LOCATION > (aka zero, which it already is), not input_location, in make_node. I sort of think it makes sense to move all the tree class specific bits out of make_node to functions for that specific type of tree, but it is mostly unrelated. One advantage of that is that it saves pointless initialization in the module / lto streamer that gets over written with the streamed values. However having used the argument to find all the places that create decls, and having updated them, while the argument and asserts do prevent leaving the location uninitialized by mistake, I'd be fine with dropping that part and just updating all the make_node callers to use build_decl. thanks Trev > > Richard. > > > Trev > > > > gcc/ChangeLog: > > > > * cfgexpand.c (avoid_deep_ter_for_debug): Call build_decl not > > make_node. > > (expand_gimple_basic_block): Likewise. > > * ipa-param-manipulation.c (ipa_param_adjustments::modify_call): > > * Likewise. > > (ipa_param_body_adjustments::reset_debug_stmts): Likewise. > > * omp-simd-clone.c (ipa_simd_modify_stmt_ops): Likewise. > > * stor-layout.c (start_bitfield_representative): Likewise. > > * tree-inline.c (remap_ssa_name): Likewise. > > (tree_function_versioning): Likewise. > > * tree-into-ssa.c (rewrite_debug_stmt_uses): Likewise. > > * tree-nested.c (lookup_field_for_decl): Likewise. > > (get_chain_field): Likewise. > > (create_field_for_decl): Likewise. > > (get_nl_goto_field): Likewise. > > (finalize_nesting_tree_1): Likewise. > > * tree-ssa-ccp.c (optimize_atomic_bit_test_and): Likewise. > > * tree-ssa-loop-ivopts.c (remove_unused_ivs): Likewise. > > * tree-ssa-phiopt.c (spaceship_replacement): Likewise. > > * tree-ssa-reassoc.c (make_new_ssa_for_def): Likewise. > > * tree-ssa.c (insert_debug_temp_for_var_def): Likewise. > > * tree-streamer-in.c (streamer_alloc_tree): Adjust. > > * tree.c (make_node): Add argument to specify the caller. > > (build_decl): Move initialization from make_node. > > * tree.h (enum make_node_caller): new enum. > > (make_node): Adjust prototype. > > * varasm.c (make_debug_expr_from_rtl): call build_decl. > > > > gcc/cp/ChangeLog: > > > > * constraint.cc (build_type_constraint): Call build_decl not > > make_node. > > * cp-gimplify.c (cp_genericize_r): Likewise. > > * parser.c (cp_parser_introduction_list): Likewise. > > * module.cc (trees_in::start): Adjust. > > > > gcc/fortran/ChangeLog: > > > > * trans-decl.c (generate_namelist_decl): Call build_decl not > > make_node. > > * trans-types.c (gfc_get_array_descr_info): Likewise. > > > > gcc/objc/ChangeLog: > > > > * objc-act.c (objc_add_property_declaration): Call build_decl not > > make_node. > > (maybe_make_artificial_property_decl): Likewise. > > (objc_build_keyword_decl): Likewise. > > (build_method_decl): Likewise. > > --- >
Re: [PATCH 1/4] force decls to be allocated through build_decl to initialize them
On Wed, Jul 14, 2021 at 10:20 AM Trevor Saunders wrote: > > prior to this commit all calls to build_decl used input_location, even if > temporarily until build_decl reset the location to something else that it was > told was the proper location. To avoid using the global we need the caller to > pass in the location it wants, however that's not possible with make_node > since > it makes other types of nodes. So we force all callers who wish to make a > decl > to go through build_decl which already takes a location argument. To avoid > changing behavior this just explicitly passes in input_location to build_decl > for callers of make_node that create a decl, however it would seem in many of > these cases that the location of the decl being coppied might be a better > location. > > bootstrapped and regtested on x86_64-linux-gnu, ok? I think all eventually DECL_ARTIFICIAL decls should better use UNKNOWN_LOCATION instead of input_location. I'm not sure if I like the (transitional) extra arg to make_node, I suppose we could hide make_node by declaring it in tree-raw.h or so or by guarding the decl with NEED_MAKE_NODE. There's nothing inherently wrong with calling make_node. So what I mean with transitional is that with this change we should simply set the location to UNKNOWN_LOCATION (aka zero, which it already is), not input_location, in make_node. Richard. > Trev > > gcc/ChangeLog: > > * cfgexpand.c (avoid_deep_ter_for_debug): Call build_decl not > make_node. > (expand_gimple_basic_block): Likewise. > * ipa-param-manipulation.c (ipa_param_adjustments::modify_call): > * Likewise. > (ipa_param_body_adjustments::reset_debug_stmts): Likewise. > * omp-simd-clone.c (ipa_simd_modify_stmt_ops): Likewise. > * stor-layout.c (start_bitfield_representative): Likewise. > * tree-inline.c (remap_ssa_name): Likewise. > (tree_function_versioning): Likewise. > * tree-into-ssa.c (rewrite_debug_stmt_uses): Likewise. > * tree-nested.c (lookup_field_for_decl): Likewise. > (get_chain_field): Likewise. > (create_field_for_decl): Likewise. > (get_nl_goto_field): Likewise. > (finalize_nesting_tree_1): Likewise. > * tree-ssa-ccp.c (optimize_atomic_bit_test_and): Likewise. > * tree-ssa-loop-ivopts.c (remove_unused_ivs): Likewise. > * tree-ssa-phiopt.c (spaceship_replacement): Likewise. > * tree-ssa-reassoc.c (make_new_ssa_for_def): Likewise. > * tree-ssa.c (insert_debug_temp_for_var_def): Likewise. > * tree-streamer-in.c (streamer_alloc_tree): Adjust. > * tree.c (make_node): Add argument to specify the caller. > (build_decl): Move initialization from make_node. > * tree.h (enum make_node_caller): new enum. > (make_node): Adjust prototype. > * varasm.c (make_debug_expr_from_rtl): call build_decl. > > gcc/cp/ChangeLog: > > * constraint.cc (build_type_constraint): Call build_decl not > make_node. > * cp-gimplify.c (cp_genericize_r): Likewise. > * parser.c (cp_parser_introduction_list): Likewise. > * module.cc (trees_in::start): Adjust. > > gcc/fortran/ChangeLog: > > * trans-decl.c (generate_namelist_decl): Call build_decl not > make_node. > * trans-types.c (gfc_get_array_descr_info): Likewise. > > gcc/objc/ChangeLog: > > * objc-act.c (objc_add_property_declaration): Call build_decl not > make_node. > (maybe_make_artificial_property_decl): Likewise. > (objc_build_keyword_decl): Likewise. > (build_method_decl): Likewise. > --- > gcc/cfgexpand.c | 8 > gcc/cp/constraint.cc | 2 +- > gcc/cp/cp-gimplify.c | 5 +++-- > gcc/cp/module.cc | 2 +- > gcc/cp/parser.c | 6 ++ > gcc/fortran/trans-decl.c | 5 ++--- > gcc/fortran/trans-types.c| 4 ++-- > gcc/ipa-param-manipulation.c | 8 > gcc/objc/objc-act.c | 16 ++-- > gcc/omp-simd-clone.c | 4 ++-- > gcc/stor-layout.c| 2 +- > gcc/tree-inline.c| 13 +++-- > gcc/tree-into-ssa.c | 4 ++-- > gcc/tree-nested.c| 24 ++-- > gcc/tree-ssa-ccp.c | 4 ++-- > gcc/tree-ssa-loop-ivopts.c | 4 ++-- > gcc/tree-ssa-phiopt.c| 8 > gcc/tree-ssa-reassoc.c | 4 ++-- > gcc/tree-ssa.c | 4 ++-- > gcc/tree-streamer-in.c | 2 +- > gcc/tree.c | 35 ++- > gcc/tree.h | 13 - > gcc/varasm.c | 12 ++-- > 23 files changed, 96 insertions(+), 93 deletions(-) > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c > index 3edd53c37dc..fea8c837c80 100644 > --- a/gcc/cfgexpand.c > +++ b/gcc/cfgexpand.c > @@ -4342,10 +4342,10 @@ avoid_deep_ter_for_debug (gimple *stmt, int
[PATCH 1/4] force decls to be allocated through build_decl to initialize them
prior to this commit all calls to build_decl used input_location, even if temporarily until build_decl reset the location to something else that it was told was the proper location. To avoid using the global we need the caller to pass in the location it wants, however that's not possible with make_node since it makes other types of nodes. So we force all callers who wish to make a decl to go through build_decl which already takes a location argument. To avoid changing behavior this just explicitly passes in input_location to build_decl for callers of make_node that create a decl, however it would seem in many of these cases that the location of the decl being coppied might be a better location. bootstrapped and regtested on x86_64-linux-gnu, ok? Trev gcc/ChangeLog: * cfgexpand.c (avoid_deep_ter_for_debug): Call build_decl not make_node. (expand_gimple_basic_block): Likewise. * ipa-param-manipulation.c (ipa_param_adjustments::modify_call): * Likewise. (ipa_param_body_adjustments::reset_debug_stmts): Likewise. * omp-simd-clone.c (ipa_simd_modify_stmt_ops): Likewise. * stor-layout.c (start_bitfield_representative): Likewise. * tree-inline.c (remap_ssa_name): Likewise. (tree_function_versioning): Likewise. * tree-into-ssa.c (rewrite_debug_stmt_uses): Likewise. * tree-nested.c (lookup_field_for_decl): Likewise. (get_chain_field): Likewise. (create_field_for_decl): Likewise. (get_nl_goto_field): Likewise. (finalize_nesting_tree_1): Likewise. * tree-ssa-ccp.c (optimize_atomic_bit_test_and): Likewise. * tree-ssa-loop-ivopts.c (remove_unused_ivs): Likewise. * tree-ssa-phiopt.c (spaceship_replacement): Likewise. * tree-ssa-reassoc.c (make_new_ssa_for_def): Likewise. * tree-ssa.c (insert_debug_temp_for_var_def): Likewise. * tree-streamer-in.c (streamer_alloc_tree): Adjust. * tree.c (make_node): Add argument to specify the caller. (build_decl): Move initialization from make_node. * tree.h (enum make_node_caller): new enum. (make_node): Adjust prototype. * varasm.c (make_debug_expr_from_rtl): call build_decl. gcc/cp/ChangeLog: * constraint.cc (build_type_constraint): Call build_decl not make_node. * cp-gimplify.c (cp_genericize_r): Likewise. * parser.c (cp_parser_introduction_list): Likewise. * module.cc (trees_in::start): Adjust. gcc/fortran/ChangeLog: * trans-decl.c (generate_namelist_decl): Call build_decl not make_node. * trans-types.c (gfc_get_array_descr_info): Likewise. gcc/objc/ChangeLog: * objc-act.c (objc_add_property_declaration): Call build_decl not make_node. (maybe_make_artificial_property_decl): Likewise. (objc_build_keyword_decl): Likewise. (build_method_decl): Likewise. --- gcc/cfgexpand.c | 8 gcc/cp/constraint.cc | 2 +- gcc/cp/cp-gimplify.c | 5 +++-- gcc/cp/module.cc | 2 +- gcc/cp/parser.c | 6 ++ gcc/fortran/trans-decl.c | 5 ++--- gcc/fortran/trans-types.c| 4 ++-- gcc/ipa-param-manipulation.c | 8 gcc/objc/objc-act.c | 16 ++-- gcc/omp-simd-clone.c | 4 ++-- gcc/stor-layout.c| 2 +- gcc/tree-inline.c| 13 +++-- gcc/tree-into-ssa.c | 4 ++-- gcc/tree-nested.c| 24 ++-- gcc/tree-ssa-ccp.c | 4 ++-- gcc/tree-ssa-loop-ivopts.c | 4 ++-- gcc/tree-ssa-phiopt.c| 8 gcc/tree-ssa-reassoc.c | 4 ++-- gcc/tree-ssa.c | 4 ++-- gcc/tree-streamer-in.c | 2 +- gcc/tree.c | 35 ++- gcc/tree.h | 13 - gcc/varasm.c | 12 ++-- 23 files changed, 96 insertions(+), 93 deletions(-) diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 3edd53c37dc..fea8c837c80 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -4342,10 +4342,10 @@ avoid_deep_ter_for_debug (gimple *stmt, int depth) tree = deep_ter_debug_map->get_or_insert (use); if (vexpr != NULL) continue; - vexpr = make_node (DEBUG_EXPR_DECL); + vexpr = build_decl (input_location, DEBUG_EXPR_DECL, nullptr, + TREE_TYPE (use)); gimple *def_temp = gimple_build_debug_bind (vexpr, use, g); DECL_ARTIFICIAL (vexpr) = 1; - TREE_TYPE (vexpr) = TREE_TYPE (use); SET_DECL_MODE (vexpr, TYPE_MODE (TREE_TYPE (use))); gimple_stmt_iterator gsi = gsi_for_stmt (g); gsi_insert_after (, def_temp, GSI_NEW_STMT); @@ -5899,14 +5899,14 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls) temporary. */ gimple *debugstmt;
