Message ID | 3e7ecb64-cd96-47a3-cb75-53e41317d90c@acm.org |
---|---|
State | New |
Headers | show |
Series | [PR/82546] tree node size | expand |
On October 13, 2017 8:29:40 PM GMT+02:00, Nathan Sidwell <nathan@acm.org> wrote: >[Although I filed this as a middle-end bug, it's really a core infra >bug, not sure who the best reviewer is] > >In working on tree streaming in the modules branch, I discovered poor >tree node size and hierarchy bits. > >Here's a fix for the first part of that. tree.c (tree_code_size) >returns sizeof (tree_type_non_common) for any tcc_type node. That's >wasteful, given we have tree_type_common-> >tree_type_with_lang_specific-> tree_type_non_common available as >choices. It's also obscuring defects in (at least) the c++ FE where we > >use tree_type_non_common fields, but claim the node doesn't contain >that >structure. Ew. > >This patch makes tree_code_size ask the lang hook for the size of >non-core type nodes. It also fixes the c++ and objc FEs to return a >size for the nodes it cares about. > >I don't (yet) know whether all the core types are tree_type_non_common, > >nor whether the FE's can return smaller sizes than the do with this >patch. But at least the control flow is now correct. during >developing >this patch I added an assert that the lang hook was returning a size at > >least as big as tree_type_non_common, so they couldn't be more broken >than before the patch. > >I intend to continue cleaning this up of course. It's not clear to me >whether we should cache these node sizes in an array, and the way it >goes about checking nodes with nested switches is understandable, but >possible not the fastest solution. However let's at least get the >sizing >right first. We were conservative exactly to avoid the langhook here. I think there's similar 'bug' on the decl side. Richard. >ok? > >nathan
On 10/16/2017 02:49 AM, Richard Biener wrote: > On October 13, 2017 8:29:40 PM GMT+02:00, Nathan Sidwell <nathan@acm.org> wrote: >> I intend to continue cleaning this up of course. It's not clear to me >> whether we should cache these node sizes in an array, and the way it >> goes about checking nodes with nested switches is understandable, but >> possible not the fastest solution. However let's at least get the >> sizing >> right first. > > We were conservative exactly to avoid the langhook here. I think there's similar 'bug' on the decl side. The other code types (decls, exprs, etc) call the langhook. tcc_type seems the exception (now?). nathan
On Mon, 16 Oct 2017, Nathan Sidwell wrote: > On 10/16/2017 02:49 AM, Richard Biener wrote: > > On October 13, 2017 8:29:40 PM GMT+02:00, Nathan Sidwell <nathan@acm.org> > > wrote: > > > > I intend to continue cleaning this up of course. It's not clear to me > > > whether we should cache these node sizes in an array, and the way it > > > goes about checking nodes with nested switches is understandable, but > > > possible not the fastest solution. However let's at least get the > > > sizing > > > right first. > > > > We were conservative exactly to avoid the langhook here. I think there's > > similar 'bug' on the decl side. > > The other code types (decls, exprs, etc) call the langhook. tcc_type seems > the exception (now?). Sorry for not looking at the patch before replying. The patch looks ok but shouldn't LANG_TYPE be also handled by the FE? LANG_TYPE itself is an odd beast if I may say that - it's only used by the C++ and Ada FEs and the Ada FE does only /* Make a dummy type corresponding to GNAT_TYPE. */ tree make_dummy_type (Entity_Id gnat_type) { ... /* Create a debug type so that debug info consumers only see an unspecified type. */ if (Needs_Debug_Info (gnat_type)) { debug_type = make_node (LANG_TYPE); SET_TYPE_DEBUG_TYPE (gnu_type, debug_type); TYPE_NAME (debug_type) = TYPE_NAME (gnu_type); TYPE_ARTIFICIAL (debug_type) = TYPE_ARTIFICIAL (gnu_type); } Thus the patch is ok. Thanks, Richard.
On 10/17/2017 05:26 AM, Richard Biener wrote: > Sorry for not looking at the patch before replying. The patch looks ok > but shouldn't LANG_TYPE be also handled by the FE? LANG_TYPE itself > is an odd beast if I may say that - it's only used by the C++ and Ada FEs > and the Ada FE does only I agree. I think LANG_TYPE may be from when there were no FE-specific nodes. It should probably be killed and resurrected as appropriate FE nodes. Olivier, as an ADA person, is that something that could be done? > Thus the patch is ok. Thanks, As a heads up, we currently have: struct type_common; struct type_with_lang_specific : type_common; struct type_non_common : type_with_lang_specific; And many (most?, all?) FE type nodes derive from type_non_common (even if, as I discovered, they don't know it). It seems the hierarchy would be better as: struct type_common; struct type_non_common : type_common; // FE type derive here struct type_with_lang_specific : type_non_common; After all, why would a FE-specific type need a lang-specific pointer? I don't think there are types that just have the land-pointer and don't have non_common. That's the direction I'm heading in with this clean up. nathan
On Tue, 17 Oct 2017, Nathan Sidwell wrote: > On 10/17/2017 05:26 AM, Richard Biener wrote: > > > Sorry for not looking at the patch before replying. The patch looks ok > > but shouldn't LANG_TYPE be also handled by the FE? LANG_TYPE itself > > is an odd beast if I may say that - it's only used by the C++ and Ada FEs > > and the Ada FE does only > > I agree. I think LANG_TYPE may be from when there were no FE-specific nodes. > It should probably be killed and resurrected as appropriate FE nodes. > > Olivier, as an ADA person, is that something that could be done? > > > Thus the patch is ok. > > Thanks, > > As a heads up, we currently have: > > struct type_common; > struct type_with_lang_specific : type_common; > struct type_non_common : type_with_lang_specific; > > And many (most?, all?) FE type nodes derive from type_non_common (even if, as > I discovered, they don't know it). It seems the hierarchy would be better as: > > struct type_common; > struct type_non_common : type_common; // FE type derive here > struct type_with_lang_specific : type_non_common; > > After all, why would a FE-specific type need a lang-specific pointer? I don't > think there are types that just have the land-pointer and don't have > non_common. > > That's the direction I'm heading in with this clean up. Not sure. For decls the lang_specific pointer is even in decl_common! It's probably that there are basically no types w/o a FE using the lang-specific pointer but there are some types using type_common fields only (but need lang-specifics). So maybe do like with decls and remove type_with_lang_specific, instead folding it into type_common? Richard.
> On Oct 17, 2017, at 16:10 , Nathan Sidwell <nathan@acm.org> wrote: > > On 10/17/2017 05:26 AM, Richard Biener wrote: > >> Sorry for not looking at the patch before replying. The patch looks ok >> but shouldn't LANG_TYPE be also handled by the FE? LANG_TYPE itself >> is an odd beast if I may say that - it's only used by the C++ and Ada FEs >> and the Ada FE does only > > I agree. I think LANG_TYPE may be from when there were no FE-specific nodes. It should probably be killed and resurrected as appropriate FE nodes. > > Olivier, as an ADA person, is that something that could be done? I'd think so. LANG_TYPE is treated specially in several places and Ada debug types are pretty sensitive so this would require caution but I don't see/know-of obvious reasons why this couldn't be done. Eric and Pierre-Marie (cc'ed) might have more precise insights. Olivier
> I'd think so. LANG_TYPE is treated specially in several > places and Ada debug types are pretty sensitive so this would > require caution but I don't see/know-of obvious reasons why this > couldn't be done. LANG_TYPE is only used in Ada to trigger the specific treatment in gen_type_die_with_usage for DW_TAG_unspecified_type, so very minor.
> On 18 Oct 2017, at 15:59, Eric Botcazou <ebotcazou@adacore.com> wrote: > >> I'd think so. LANG_TYPE is treated specially in several >> places and Ada debug types are pretty sensitive so this would >> require caution but I don't see/know-of obvious reasons why this >> couldn't be done. > > LANG_TYPE is only used in Ada to trigger the specific treatment in > gen_type_die_with_usage for DW_TAG_unspecified_type, so very minor. OK, thanks for confirming Eric!
2017-10-13 Nathan Sidwell <nathan@acm.org> PR middle-end/82546 gcc/ * tree.c (tree_code_size): Reformat. Punt to lang hook for unknown TYPE nodes. gcc/cp/ * cp-objcp-common.c (cp_tree_size): Reformat. Adjust returns size of TYPE nodes. gcc/objc/ * objc-act.c (objc_common_tree_size): Return size of TYPE nodes. Index: cp/cp-objcp-common.c =================================================================== --- cp/cp-objcp-common.c (revision 253733) +++ cp/cp-objcp-common.c (working copy) @@ -61,43 +61,34 @@ cxx_warn_unused_global_decl (const_tree size_t cp_tree_size (enum tree_code code) { + gcc_checking_assert (code >= NUM_TREE_CODES); switch (code) { - case PTRMEM_CST: return sizeof (struct ptrmem_cst); - case BASELINK: return sizeof (struct tree_baselink); + case PTRMEM_CST: return sizeof (ptrmem_cst); + case BASELINK: return sizeof (tree_baselink); case TEMPLATE_PARM_INDEX: return sizeof (template_parm_index); - case DEFAULT_ARG: return sizeof (struct tree_default_arg); - case DEFERRED_NOEXCEPT: return sizeof (struct tree_deferred_noexcept); - case OVERLOAD: return sizeof (struct tree_overload); - case STATIC_ASSERT: return sizeof (struct tree_static_assert); + case DEFAULT_ARG: return sizeof (tree_default_arg); + case DEFERRED_NOEXCEPT: return sizeof (tree_deferred_noexcept); + case OVERLOAD: return sizeof (tree_overload); + case STATIC_ASSERT: return sizeof (tree_static_assert); case TYPE_ARGUMENT_PACK: - case TYPE_PACK_EXPANSION: - return sizeof (struct tree_common); - + case TYPE_PACK_EXPANSION: return sizeof (tree_type_non_common); case NONTYPE_ARGUMENT_PACK: - case EXPR_PACK_EXPANSION: - return sizeof (struct tree_exp); - - case ARGUMENT_PACK_SELECT: - return sizeof (struct tree_argument_pack_select); - - case TRAIT_EXPR: - return sizeof (struct tree_trait_expr); - - case LAMBDA_EXPR: return sizeof (struct tree_lambda_expr); - - case TEMPLATE_INFO: return sizeof (struct tree_template_info); - - case CONSTRAINT_INFO: return sizeof (struct tree_constraint_info); - - case USERDEF_LITERAL: return sizeof (struct tree_userdef_literal); - - case TEMPLATE_DECL: return sizeof (struct tree_template_decl); - + case EXPR_PACK_EXPANSION: return sizeof (tree_exp); + case ARGUMENT_PACK_SELECT: return sizeof (tree_argument_pack_select); + case TRAIT_EXPR: return sizeof (tree_trait_expr); + case LAMBDA_EXPR: return sizeof (tree_lambda_expr); + case TEMPLATE_INFO: return sizeof (tree_template_info); + case CONSTRAINT_INFO: return sizeof (tree_constraint_info); + case USERDEF_LITERAL: return sizeof (tree_userdef_literal); + case TEMPLATE_DECL: return sizeof (tree_template_decl); default: - if (TREE_CODE_CLASS (code) == tcc_declaration) - return sizeof (struct tree_decl_non_common); - gcc_unreachable (); + switch (TREE_CODE_CLASS (code)) + { + case tcc_declaration: return sizeof (tree_decl_non_common); + case tcc_type: return sizeof (tree_type_non_common); + default: gcc_unreachable (); + } } /* NOTREACHED */ } Index: objc/objc-act.c =================================================================== --- objc/objc-act.c (revision 253733) +++ objc/objc-act.c (working copy) @@ -10118,11 +10118,14 @@ objc_common_tree_size (enum tree_code co case CLASS_METHOD_DECL: case INSTANCE_METHOD_DECL: case KEYWORD_DECL: - case PROPERTY_DECL: - return sizeof (struct tree_decl_non_common); + case PROPERTY_DECL: return sizeof (tree_decl_non_common); + case CLASS_INTERFACE_TYPE: + case CLASS_IMPLEMENTATION_TYPE: + case CATEGORY_INTERFACE_TYPE: + case CATEGORY_IMPLEMENTATION_TYPE: + case PROTOCOL_INTERFACE_TYPE: return sizeof (tree_type_non_common); default: gcc_unreachable (); - } } Index: tree.c =================================================================== --- tree.c (revision 253733) +++ tree.c (working copy) @@ -763,40 +763,53 @@ tree_code_size (enum tree_code code) switch (TREE_CODE_CLASS (code)) { case tcc_declaration: /* A decl node */ - { - switch (code) - { - case FIELD_DECL: - return sizeof (struct tree_field_decl); - case PARM_DECL: - return sizeof (struct tree_parm_decl); - case VAR_DECL: - return sizeof (struct tree_var_decl); - case LABEL_DECL: - return sizeof (struct tree_label_decl); - case RESULT_DECL: - return sizeof (struct tree_result_decl); - case CONST_DECL: - return sizeof (struct tree_const_decl); - case TYPE_DECL: - return sizeof (struct tree_type_decl); - case FUNCTION_DECL: - return sizeof (struct tree_function_decl); - case DEBUG_EXPR_DECL: - return sizeof (struct tree_decl_with_rtl); - case TRANSLATION_UNIT_DECL: - return sizeof (struct tree_translation_unit_decl); - case NAMESPACE_DECL: - case IMPORTED_DECL: - case NAMELIST_DECL: - return sizeof (struct tree_decl_non_common); - default: - return lang_hooks.tree_size (code); - } - } + switch (code) + { + case FIELD_DECL: return sizeof (tree_field_decl); + case PARM_DECL: return sizeof (tree_parm_decl); + case VAR_DECL: return sizeof (tree_var_decl); + case LABEL_DECL: return sizeof (tree_label_decl); + case RESULT_DECL: return sizeof (tree_result_decl); + case CONST_DECL: return sizeof (tree_const_decl); + case TYPE_DECL: return sizeof (tree_type_decl); + case FUNCTION_DECL: return sizeof (tree_function_decl); + case DEBUG_EXPR_DECL: return sizeof (tree_decl_with_rtl); + case TRANSLATION_UNIT_DECL: return sizeof (tree_translation_unit_decl); + case NAMESPACE_DECL: + case IMPORTED_DECL: + case NAMELIST_DECL: return sizeof (tree_decl_non_common); + default: + gcc_checking_assert (code >= NUM_TREE_CODES); + return lang_hooks.tree_size (code); + } case tcc_type: /* a type node */ - return sizeof (struct tree_type_non_common); + switch (code) + { + case OFFSET_TYPE: + case ENUMERAL_TYPE: + case BOOLEAN_TYPE: + case INTEGER_TYPE: + case REAL_TYPE: + case POINTER_TYPE: + case REFERENCE_TYPE: + case NULLPTR_TYPE: + case FIXED_POINT_TYPE: + case COMPLEX_TYPE: + case VECTOR_TYPE: + case ARRAY_TYPE: + case RECORD_TYPE: + case UNION_TYPE: + case QUAL_UNION_TYPE: + case VOID_TYPE: + case POINTER_BOUNDS_TYPE: + case FUNCTION_TYPE: + case METHOD_TYPE: + case LANG_TYPE: return sizeof (tree_type_non_common); + default: + gcc_checking_assert (code >= NUM_TREE_CODES); + return lang_hooks.tree_size (code); + } case tcc_reference: /* a reference */ case tcc_expression: /* an expression */ @@ -810,14 +823,15 @@ tree_code_size (enum tree_code code) case tcc_constant: /* a constant */ switch (code) { - case VOID_CST: return sizeof (struct tree_typed); + case VOID_CST: return sizeof (tree_typed); case INTEGER_CST: gcc_unreachable (); - case REAL_CST: return sizeof (struct tree_real_cst); - case FIXED_CST: return sizeof (struct tree_fixed_cst); - case COMPLEX_CST: return sizeof (struct tree_complex); - case VECTOR_CST: return sizeof (struct tree_vector); + case REAL_CST: return sizeof (tree_real_cst); + case FIXED_CST: return sizeof (tree_fixed_cst); + case COMPLEX_CST: return sizeof (tree_complex); + case VECTOR_CST: return sizeof (tree_vector); case STRING_CST: gcc_unreachable (); default: + gcc_checking_assert (code >= NUM_TREE_CODES); return lang_hooks.tree_size (code); } @@ -825,23 +839,24 @@ tree_code_size (enum tree_code code) switch (code) { case IDENTIFIER_NODE: return lang_hooks.identifier_size; - case TREE_LIST: return sizeof (struct tree_list); + case TREE_LIST: return sizeof (tree_list); case ERROR_MARK: - case PLACEHOLDER_EXPR: return sizeof (struct tree_common); + case PLACEHOLDER_EXPR: return sizeof (tree_common); - case TREE_VEC: + case TREE_VEC: gcc_unreachable (); case OMP_CLAUSE: gcc_unreachable (); - case SSA_NAME: return sizeof (struct tree_ssa_name); + case SSA_NAME: return sizeof (tree_ssa_name); - case STATEMENT_LIST: return sizeof (struct tree_statement_list); + case STATEMENT_LIST: return sizeof (tree_statement_list); case BLOCK: return sizeof (struct tree_block); - case CONSTRUCTOR: return sizeof (struct tree_constructor); - case OPTIMIZATION_NODE: return sizeof (struct tree_optimization_option); - case TARGET_OPTION_NODE: return sizeof (struct tree_target_option); + case CONSTRUCTOR: return sizeof (tree_constructor); + case OPTIMIZATION_NODE: return sizeof (tree_optimization_option); + case TARGET_OPTION_NODE: return sizeof (tree_target_option); default: + gcc_checking_assert (code >= NUM_TREE_CODES); return lang_hooks.tree_size (code); }