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 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
On Wed, Apr 20, 2011 at 12:00 AM, Nathan Froyd froy...@codesourcery.com 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
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 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 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
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