Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL

Re: [PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL

[Steven Bosscher]

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

[Nathan Froyd]

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

[Michael Matz]

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

[Nathan Froyd]

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

[Nathan Froyd]

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

[Richard Guenther]

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

[Michael Matz]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1679 bytes --]

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

[Nathan Froyd]

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

[PATCH] make LABEL_DECL has its own rtx field for its associated CODE_LABEL

[Nathan Froyd]

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;
 };