Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
On Thu, Apr 21, 2011 at 05:54:28PM +0200, Michael Matz wrote: > > > In particular, FIELD_DECLs have a size, but they have no RTL associated > > > with them. And LABEL_DECLs have RTL, but no size. > > Blaeh. So far about nice clean ideas :) One hacky idea: change my > proposal to this: > > decl_common {} # no size, no rtl, no align, no pt_uid > decl_with_rtl_or_size : decl_common { > # add align, pt_uid >union { > rtx rtl; > tree size; >} u; > } > decl_with_size : decl_with_rtl_or_size { > # add size, size_unit > } > > Use the rtl_or_size struct primarily for the current _with_rtl things that > don't naturally have a size, but use it also for FIELD_DECLs via the > union. I'm not sure I follow this wrt FIELD_DECLs. Before you have: ...lots of decl fields.. tree size; tree size_unit; After you have: ...lots of decl fields... union { rtx rtl; tree size; } u; tree size; tree size_unit; Or did you mean something like: /* As above. */ decl_with_rtl_or_size /* Add a size_unit field. */ decl_with_size_unit : decl_with_rtl_or_size /* FIELD_DECL */ /* Add a size field for DECLs that do have RTL. */ decl_with_rtl_and_size : decl_with_size_unit /* VAR_DECL, PARM_DECL, etc. */ which looks just awful. :) > Alternatively I could also envision making a new tree_ struct for just > field_decls, that would contain the size and other fields that currently > are in decl_common just for fields (in particular the off_align) member. > The various accessors like DECL_SIZE would then need to dispatch based on > tree code. You could also do something like: struct tree_decl_size { tree size; tree size_unit; ... }; struct tree_field_decl { ... struct tree_decl_size size; }; struct tree_var_decl { ... struct tree_decl_size size; }; static inline tree * decl_size_1 (tree node) { switch (TREE_CODE (node)) { case FIELD_DECL: return &node->field_decl.size.size; case VAR_DECL: return &node->var_decl.size.size; ... default: gcc_unreachable (); } } /* Might also need a HAS_DECL_SIZE_P predicate or similar. */ #define DECL_SIZE(NODE) (*decl_size_1 (NODE)) ... which would make it somewhat more obvious when things have sizes, as well as letting you remove DECL_SIZE{,_UNIT} from FUNCTION_DECL. Slimming CONST_DECL and LABEL_DECL like this is useful mostly for code cleanliness, but eliminating such fields from FUNCTION_DECL would have a much more noticeable memory size impact. -Nathan
Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
Hi, On Wed, 20 Apr 2011, Richard Guenther wrote: > > I had occasion to try this today; this inheritance structure doesn't > > work. The truncated inheritance tree looks like: > > > > * decl_common > > * field_decl > > * const_decl > > * decl_with_rtl > > * label_decl > > * result_decl > > * parm_decl > > * decl_with_vis... > > > > In particular, FIELD_DECLs have a size, but they have no RTL associated > > with them. And LABEL_DECLs have RTL, but no size. Blaeh. So far about nice clean ideas :) One hacky idea: change my proposal to this: decl_common {} # no size, no rtl, no align, no pt_uid decl_with_rtl_or_size : decl_common { # add align, pt_uid union { rtx rtl; tree size; } u; } decl_with_size : decl_with_rtl_or_size { # add size, size_unit } Use the rtl_or_size struct primarily for the current _with_rtl things that don't naturally have a size, but use it also for FIELD_DECLs via the union. Alternatively I could also envision making a new tree_ struct for just field_decls, that would contain the size and other fields that currently are in decl_common just for fields (in particular the off_align) member. The various accessors like DECL_SIZE would then need to dispatch based on tree code. Also doesn't sound terribly sexy. FWIW I'm usually against on the side mappings A->B if most As will most of the time be associated with a B. A size _is_ associated with the entity always (for entities where it makes sense to talk about sizes), so that's exactly where I would find on the side tables strange. For DECL_RTL it's less clear. Ciao, Michael.
Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
On Wed, Apr 20, 2011 at 12:00 AM, Nathan Froyd wrote: > On Tue, Apr 05, 2011 at 05:55:33PM +0200, Michael Matz wrote: >> I have a preference in having just one DECL_RTL field for conceptual >> reasons: >> >> Most DECLs are actually objects (there are some prominent exceptions, but >> those always would be better described with something like NAMED_ENTITY, >> if we had something like that, namespaces and translation_unit would >> qualify). All these have a RTL representation, so one field for them >> seems appropriate. That some of those don't have a size (either because >> size makes no sense or is always available via type size) hints towards a >> problem in the inheritance. I would think it should look like so: >> >> decl_common {} # no size, no rtl, no align, no pt_uid >> decl_with_rtl : decl_common { >> # add rtl, align, pt_uid >> } >> decl_with_size : decl_with_rtl { >> # add size, size_unit >> } >> >> Then decl_common can still be used for >> imported_decl/namespace/translation_unit; objects >> are at least decl_with_rtl, and some objects will be decl_with_size. > > I had occasion to try this today; this inheritance structure doesn't > work. The truncated inheritance tree looks like: > > * decl_common > * field_decl > * const_decl > * decl_with_rtl > * label_decl > * result_decl > * parm_decl > * decl_with_vis... > > In particular, FIELD_DECLs have a size, but they have no RTL associated > with them. And LABEL_DECLs have RTL, but no size. So if you went with > the above, FIELD_DECLs would grow by one (useless) word. And the > reverse is the situation we have today, where CONST_DECLs and > LABEL_DECLs (at least) have a pointless DECL_SIZE. Ideally, we could > fix things like FUNCTION_DECLs having a size, too... > > And I didn't check the C++ FE to see if there are problematic cases > there, either. > > What do you think is the next step? To address this issue, we could > just give LABEL_DECL its own rtx field as in the original patch, and > that would resolve that. But maybe we should go further, say by making > DECL_SIZE{,_UNIT} and/or DECL_RTL into actual (out-of-line function) > accessors; these accessors can then access structure-specific bits of > data. Then we don't have to worry about the inheritance structure, and > maybe could adopt alternate storage schemes for different DECLs, such as > the off-to-the-side table that Steven suggested. Another option is to change nothing ;) Conceptually I'd say not storing DECL_RTL in the decls themselves but on the side would make sense, at least from a stylish view. I'm not sure it'll work out very well though in terms of cost & benefit. What we could do is, if we ever can dispose of DECL/TYPE_LANG_SPECIFIC after lowering to gimple, overload that field with a DECL/TYPE_RTL_SPECIFIC field ... Richard. > -Nathan >
Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
On Tue, Apr 05, 2011 at 05:55:33PM +0200, Michael Matz wrote: > I have a preference in having just one DECL_RTL field for conceptual > reasons: > > Most DECLs are actually objects (there are some prominent exceptions, but > those always would be better described with something like NAMED_ENTITY, > if we had something like that, namespaces and translation_unit would > qualify). All these have a RTL representation, so one field for them > seems appropriate. That some of those don't have a size (either because > size makes no sense or is always available via type size) hints towards a > problem in the inheritance. I would think it should look like so: > > decl_common {} # no size, no rtl, no align, no pt_uid > decl_with_rtl : decl_common { > # add rtl, align, pt_uid > } > decl_with_size : decl_with_rtl { > # add size, size_unit > } > > Then decl_common can still be used for > imported_decl/namespace/translation_unit; objects > are at least decl_with_rtl, and some objects will be decl_with_size. I had occasion to try this today; this inheritance structure doesn't work. The truncated inheritance tree looks like: * decl_common * field_decl * const_decl * decl_with_rtl * label_decl * result_decl * parm_decl * decl_with_vis... In particular, FIELD_DECLs have a size, but they have no RTL associated with them. And LABEL_DECLs have RTL, but no size. So if you went with the above, FIELD_DECLs would grow by one (useless) word. And the reverse is the situation we have today, where CONST_DECLs and LABEL_DECLs (at least) have a pointless DECL_SIZE. Ideally, we could fix things like FUNCTION_DECLs having a size, too... And I didn't check the C++ FE to see if there are problematic cases there, either. What do you think is the next step? To address this issue, we could just give LABEL_DECL its own rtx field as in the original patch, and that would resolve that. But maybe we should go further, say by making DECL_SIZE{,_UNIT} and/or DECL_RTL into actual (out-of-line function) accessors; these accessors can then access structure-specific bits of data. Then we don't have to worry about the inheritance structure, and maybe could adopt alternate storage schemes for different DECLs, such as the off-to-the-side table that Steven suggested. -Nathan
Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
On Tue, Apr 05, 2011 at 05:55:33PM +0200, Michael Matz wrote: > I have a preference in having just one DECL_RTL field for conceptual > reasons: > > Most DECLs are actually objects (there are some prominent exceptions, but > those always would be better described with something like NAMED_ENTITY, > if we had something like that, namespaces and translation_unit would > qualify). All these have a RTL representation, so one field for them > seems appropriate. That some of those don't have a size (either because > size makes no sense or is always available via type size) hints towards a > problem in the inheritance. I would think it should look like so: > > decl_common {} # no size, no rtl, no align, no pt_uid > decl_with_rtl : decl_common { > # add rtl, align, pt_uid > } > decl_with_size : decl_with_rtl { > # add size, size_unit > } > > Then decl_common can still be used for > imported_decl/namespace/translation_unit; objects > are at least decl_with_rtl, and some objects will be decl_with_size. This is what I was heading for, but I hadn't fully worked out how to switch around the inheritance structure. Thanks! Consider this patch dropped, then; I'll work towards something like the above instead. -Nathan
Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
Hi, On Mon, 4 Apr 2011, Nathan Froyd wrote: > On Mon, Apr 04, 2011 at 05:52:00PM +0200, Steven Bosscher wrote: > > Have you looked into maybe putting the CODE_LABEL for a LABEL_DECL in > > an on-the-side structure (hash table, whatever)? It looks like it is > > only used during expansion of SWITCH statements. > > I haven't, though it'd be easy enough once all accesses go through > label_rtx. I'd be equally happy doing that instead of pushing DECL_RTX > down. I have a preference in having just one DECL_RTL field for conceptual reasons: Most DECLs are actually objects (there are some prominent exceptions, but those always would be better described with something like NAMED_ENTITY, if we had something like that, namespaces and translation_unit would qualify). All these have a RTL representation, so one field for them seems appropriate. That some of those don't have a size (either because size makes no sense or is always available via type size) hints towards a problem in the inheritance. I would think it should look like so: decl_common {} # no size, no rtl, no align, no pt_uid decl_with_rtl : decl_common { # add rtl, align, pt_uid } decl_with_size : decl_with_rtl { # add size, size_unit } Then decl_common can still be used for imported_decl/namespace/translation_unit; objects are at least decl_with_rtl, and some objects will be decl_with_size. Ciao, Michael. 5B
Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
On Mon, Apr 04, 2011 at 05:52:00PM +0200, Steven Bosscher wrote: > Have you looked into maybe putting the CODE_LABEL for a LABEL_DECL in > an on-the-side structure (hash table, whatever)? It looks like it is > only used during expansion of SWITCH statements. I haven't, though it'd be easy enough once all accesses go through label_rtx. I'd be equally happy doing that instead of pushing DECL_RTX down. -Nathan
Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
Hi Nathan, Have you looked into maybe putting the CODE_LABEL for a LABEL_DECL in an on-the-side structure (hash table, whatever)? It looks like it is only used during expansion of SWITCH statements. Ciao! Steven
[PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL
This patch does just what $SUBJECT suggests: pushes down the DECL_RTL field into LABEL_DECL. In this way, LABEL_DECL can inherit from tree_decl_common instead of tree_decl_with_rtl. I realize this looks like pure code shuffling; the reason for doing this is that I want to split tree_decl_common into two structures: one that includes DECL_SIZE and DECL_SIZE_UNIT and one that doesn't. The latter can then be used for CONST_DECL, LABEL_DECL, and--possibly--RESULT_DECL and--*maybe*--PARM_DECL. (Once the latter two have DECL_RTL "pushed down" as well, of course.) And I think that less multipurposing of DECL_RTL is not a bad thing. Tested on x86_64-unknown-linux-gnu. OK to commit? -Nathan * tree.h (struct tree_label_decl): Inherit from tree_decl_common. Add `label' field. (LABEL_DECL_CODE_LABEL): New macro. * stmt.c (label_rtx): Use it. (expand_label): Use `label_r', rather than fetching DECL_RTL. * tree.c (initialize_tree_contains_struct): Adjust LABEL_DECL inheritance and tree_contains_struct asserts. diff --git a/gcc/stmt.c b/gcc/stmt.c index 1a9f9e5..13a906f 100644 --- a/gcc/stmt.c +++ b/gcc/stmt.c @@ -135,17 +135,15 @@ static struct case_node *add_case_node (struct case_node *, tree, rtx label_rtx (tree label) { - gcc_assert (TREE_CODE (label) == LABEL_DECL); - - if (!DECL_RTL_SET_P (label)) + if (!LABEL_DECL_CODE_LABEL (label)) { rtx r = gen_label_rtx (); - SET_DECL_RTL (label, r); + LABEL_DECL_CODE_LABEL (label) = r; if (FORCED_LABEL (label) || DECL_NONLOCAL (label)) LABEL_PRESERVE_P (r) = 1; } - return DECL_RTL (label); + return LABEL_DECL_CODE_LABEL (label); } /* As above, but also put it on the forced-reference list of the @@ -207,7 +205,7 @@ expand_label (tree label) do_pending_stack_adjust (); emit_label (label_r); if (DECL_NAME (label)) -LABEL_NAME (DECL_RTL (label)) = IDENTIFIER_POINTER (DECL_NAME (label)); +LABEL_NAME (label_r) = IDENTIFIER_POINTER (DECL_NAME (label)); if (DECL_NONLOCAL (label)) { diff --git a/gcc/tree.c b/gcc/tree.c index ee47982..3c2154f 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -440,6 +440,7 @@ initialize_tree_contains_struct (void) case TS_DECL_WRTL: case TS_CONST_DECL: + case TS_LABEL_DECL: MARK_TS_DECL_COMMON (code); break; @@ -449,7 +450,6 @@ initialize_tree_contains_struct (void) case TS_DECL_WITH_VIS: case TS_PARM_DECL: - case TS_LABEL_DECL: case TS_RESULT_DECL: MARK_TS_DECL_WRTL (code); break; @@ -492,7 +492,6 @@ initialize_tree_contains_struct (void) gcc_assert (tree_contains_struct[PARM_DECL][TS_DECL_WRTL]); gcc_assert (tree_contains_struct[RESULT_DECL][TS_DECL_WRTL]); gcc_assert (tree_contains_struct[FUNCTION_DECL][TS_DECL_WRTL]); - gcc_assert (tree_contains_struct[LABEL_DECL][TS_DECL_WRTL]); gcc_assert (tree_contains_struct[CONST_DECL][TS_DECL_MINIMAL]); gcc_assert (tree_contains_struct[VAR_DECL][TS_DECL_MINIMAL]); gcc_assert (tree_contains_struct[PARM_DECL][TS_DECL_MINIMAL]); diff --git a/gcc/tree.h b/gcc/tree.h index fcdebd9..952e13d 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -2940,6 +2940,11 @@ struct GTY(()) tree_field_decl { #define LABEL_DECL_UID(NODE) \ (LABEL_DECL_CHECK (NODE)->label_decl.label_decl_uid) +/* The CODE_LABEL associated with a LABEL_DECL. This macro should not + be used directly; use label_rtx instead. */ +#define LABEL_DECL_CODE_LABEL(NODE) \ + (LABEL_DECL_CHECK (NODE)->label_decl.label) + /* In a LABEL_DECL, the EH region number for which the label is the post_landing_pad. */ #define EH_LANDING_PAD_NR(NODE) \ @@ -2951,7 +2956,8 @@ struct GTY(()) tree_field_decl { (LABEL_DECL_CHECK (NODE)->decl_common.decl_flag_0) struct GTY(()) tree_label_decl { - struct tree_decl_with_rtl common; + struct tree_decl_common common; + rtx label; int label_decl_uid; int eh_landing_pad_nr; };