Re: [cxx-conversion] Add Record Builder Class
On 02/12/13 19:47, Lawrence Crowl wrote: Add class record_builder to ease construction of records and unions. Use it in some appropriate places. Nathan please review the vxworks changes. config/vxworks.c Replace vxworks_emutls_var_fields() with vxworks_emutls_object_type(). Use record_builder within vxworks_emutls_object_type(). Tested on x86_64. Tested with config-list.mk on vxworks targets. The vxworks bits look fine, thanks. -- Nathan Sidwell
Re: [cxx-conversion] Add Record Builder Class
On Thu, Feb 14, 2013 at 8:44 PM, Lawrence Crowl cr...@google.com wrote:
On 2/14/13, Richard Biener richard.guent...@gmail.com wrote:
On Tue, Feb 12, 2013 at 8:47 PM, Lawrence Crowl cr...@google.com wrote:
Add class record_builder to ease construction of records and unions. Use
it
in some appropriate places.
tree
-default_emutls_var_fields (tree type, tree *name ATTRIBUTE_UNUSED)
+default_emutls_object_type (void)
{
- tree word_type_node, field, next_field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__templ),
ptr_type_node);
- DECL_CONTEXT (field) = type;
- next_field = field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__offset),
- ptr_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
- next_field = field;
-
- word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__align),
- word_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
- next_field = field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__size),
word_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
-
- return field;
+ tree word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
+ record_builder rec;
+ rec.add_field (__size, word_type_node);
+ rec.add_field (__align, word_type_node);
+ rec.add_field (__offset, ptr_type_node);
+ rec.add_field (__templ, ptr_type_node);
+ rec.layout ();
That's awkward - you want to hide the fact that layout has
to happen and that it has to happen last. Just make it a
side-effect of .as_tree ().
Sometimes you want to construct recursive types, and for that you
need .as_tree to execute before layout. This feature is used in
the patch.
Note that add_field want's to return the FIELD_DECL created,
people may want to alter it.
Do you have a use case? Until we have one, I'm not convinced that
we should widen the interface.
Note that tag_name does not allow the way C++ uses this (it can
be a TYPE_DECL).
That is what the .decl_name member function does.
Overall I'm not sure this is a good abstraction unless you manage
to make the frontends use it.
The intent is for use by the middle/back ends constructing code.
As such, it should be using middle/back end types, not front-end
types.
Note that there is no such thing as a middle-end or back-end type.
Richard.
--
Lawrence Crowl
Re: [cxx-conversion] Add Record Builder Class
On Fri, Feb 15, 2013 at 3:04 AM, Richard Biener richard.guent...@gmail.com wrote: Note that there is no such thing as a middle-end or back-end type. but we do conceptually have them. -- Gaby
Re: [cxx-conversion] Add Record Builder Class
On Tue, Feb 12, 2013 at 8:47 PM, Lawrence Crowl cr...@google.com wrote:
Add class record_builder to ease construction of records and unions. Use it
in some appropriate places.
Nathan please review the vxworks changes.
tree.h
New class record_builder.
tree.c
Implement record_builder member functions.
asan.c
Change asan_global_struct to use record_builder.
coverage.c
Change build_info_type() to use record_builder. It now takes a
record_builder as a parameter and returns the tree representing
the type.
Change build_fn_info_type() to use record_builder. It now returns
the tree representing the type.
Modify coverage_obj_init() to call them appropriately.
tree-mudflap.c
Change mf_make_mf_cache_struct_type() to use record_builder.
target.def
Replace the emutls var_fields hook with object_type hook. The
essential difference is that the hook is now responsible for full
construction of the type, not just adding fields.
targhooks.h
Replace default_emutls_var_fields() with default_emutls_object_type().
tree-emutls.c
Replace default_emutls_var_fields() with default_emutls_object_type().
Use record_builder within default_emutls_object_type().
Change get_emutls_object_type to use the new target hook.
doc/tm.texi.in
Replace TARGET_EMUTLS_VAR_FIELDS with TARGET_EMUTLS_OBJECT_TYPE.
doc/tm.texi
Replace TARGET_EMUTLS_VAR_FIELDS with TARGET_EMUTLS_OBJECT_TYPE.
config/vxworks.c
Replace vxworks_emutls_var_fields() with vxworks_emutls_object_type().
Use record_builder within vxworks_emutls_object_type().
Tested on x86_64. Tested with config-list.mk on vxworks targets.
Index: gcc/tree-emutls.c
===
--- gcc/tree-emutls.c (revision 195904)
+++ gcc/tree-emutls.c (working copy)
@@ -103,41 +103,22 @@ get_emutls_object_name (tree name)
return prefix_name (prefix, name);
}
-/* Create the fields of the type for the control variables. Ordinarily
+/* Create the type for the control variables. Ordinarily
this must match struct __emutls_object defined in emutls.c. However
this is a target hook so that VxWorks can define its own layout. */
tree
-default_emutls_var_fields (tree type, tree *name ATTRIBUTE_UNUSED)
+default_emutls_object_type (void)
{
- tree word_type_node, field, next_field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__templ), ptr_type_node);
- DECL_CONTEXT (field) = type;
- next_field = field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__offset),
- ptr_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
- next_field = field;
-
- word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__align),
- word_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
- next_field = field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__size), word_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
-
- return field;
+ tree word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
+ record_builder rec;
+ rec.add_field (__size, word_type_node);
+ rec.add_field (__align, word_type_node);
+ rec.add_field (__offset, ptr_type_node);
+ rec.add_field (__templ, ptr_type_node);
+ rec.layout ();
That's awkward - you want to hide the fact that layout has to happen
and that it has to happen last. Just make it a side-effect of
.as_tree (). Note that add_field want's to return the FIELD_DECL
created, people may want to alter it.
Note that tag_name does not allow the way C++ uses this (it can be
a TYPE_DECL).
Overall I'm not sure this is a good abstraction unless you manage to
make the frontends use it.
Richard.
+ rec.decl_name (__emutls_object);
+ return rec.as_tree ();
}
/* Initialize emulated tls object TO, which refers to TLS variable DECL and
@@ -182,24 +163,9 @@ default_emutls_var_init (tree to, tree d
static tree
get_emutls_object_type (void)
{
- tree type, type_name, field;
-
- type = emutls_object_type;
- if (type)
-return type;
-
- emutls_object_type = type = lang_hooks.types.make_type (RECORD_TYPE);
- type_name = NULL;
- field = targetm.emutls.var_fields (type, type_name);
- if (!type_name)
-type_name = get_identifier (__emutls_object);
- type_name = build_decl (UNKNOWN_LOCATION,
- TYPE_DECL, type_name, type);
- TYPE_NAME (type) = type_name;
- TYPE_FIELDS (type) = field;
- layout_type (type);
-
- return type;
+ if
Re: [cxx-conversion] Add Record Builder Class
On Thu, Feb 14, 2013 at 4:26 AM, Richard Biener richard.guent...@gmail.com wrote: Note that tag_name does not allow the way C++ uses this (it can be a TYPE_DECL). Overall I'm not sure this is a good abstraction unless you manage to make the frontends use it. I think that is a mistake. This is a pure gimple facility. The fact that it now looks somewhat similar to what front ends do is a consequence of the state of the gimple type system. We very specifically do not want to cater to front ends here. Why do you think that should be a feature of this interface? Diego.
Re: [cxx-conversion] Add Record Builder Class
On Thu, Feb 14, 2013 at 12:56 PM, Diego Novillo dnovi...@google.com wrote: On Thu, Feb 14, 2013 at 4:26 AM, Richard Biener richard.guent...@gmail.com wrote: Note that tag_name does not allow the way C++ uses this (it can be a TYPE_DECL). Overall I'm not sure this is a good abstraction unless you manage to make the frontends use it. I think that is a mistake. This is a pure gimple facility. The fact that it now looks somewhat similar to what front ends do is a consequence of the state of the gimple type system. We very specifically do not want to cater to front ends here. Why do you think that should be a feature of this interface? Because it's otherwise almost unused. No usual gimple pass builds up record types. What's the point in introducing the abstraction if most of the users cannot use it? Richard. Diego.
Re: [cxx-conversion] Add Record Builder Class
On Thu, Feb 14, 2013 at 7:52 AM, Richard Biener richard.guent...@gmail.com wrote: Because it's otherwise almost unused. No usual gimple pass builds up record types. What's the point in introducing the abstraction if most of the users cannot use it? There may be few users on the gimple side, but you are mixing two orthogonal issues. Having a similar facility for FEs may be desirable, but not *this* one. Perhaps we could have a parent class provide a more generalized set of services. Each front end could use it or derive from it for its own use. The gimple version could do the same. Could that work? Diego.
Re: [cxx-conversion] Add Record Builder Class
On Thu, Feb 14, 2013 at 2:01 PM, Diego Novillo dnovi...@google.com wrote: On Thu, Feb 14, 2013 at 7:52 AM, Richard Biener richard.guent...@gmail.com wrote: Because it's otherwise almost unused. No usual gimple pass builds up record types. What's the point in introducing the abstraction if most of the users cannot use it? There may be few users on the gimple side, but you are mixing two orthogonal issues. Having a similar facility for FEs may be desirable, but not *this* one. Perhaps we could have a parent class provide a more generalized set of services. Each front end could use it or derive from it for its own use. The gimple version could do the same. Could that work? They all share layout_type so they should be able to share the record builder. Richard. Diego.
Re: [cxx-conversion] Add Record Builder Class
On Thu, Feb 14, 2013 at 8:06 AM, Richard Biener richard.guent...@gmail.com wrote: On Thu, Feb 14, 2013 at 2:01 PM, Diego Novillo dnovi...@google.com wrote: On Thu, Feb 14, 2013 at 7:52 AM, Richard Biener richard.guent...@gmail.com wrote: Because it's otherwise almost unused. No usual gimple pass builds up record types. What's the point in introducing the abstraction if most of the users cannot use it? There may be few users on the gimple side, but you are mixing two orthogonal issues. Having a similar facility for FEs may be desirable, but not *this* one. Perhaps we could have a parent class provide a more generalized set of services. Each front end could use it or derive from it for its own use. The gimple version could do the same. Could that work? They all share layout_type so they should be able to share the record builder. That's why I was proposing a hierarchy. It's true that there is shared behaviour we want, but I'm sure that there will be services needed by FEs that are not required in gimple. Diego.
Re: [cxx-conversion] Add Record Builder Class
On 2/14/13, Richard Biener richard.guent...@gmail.com wrote:
On Tue, Feb 12, 2013 at 8:47 PM, Lawrence Crowl cr...@google.com wrote:
Add class record_builder to ease construction of records and unions. Use
it
in some appropriate places.
tree
-default_emutls_var_fields (tree type, tree *name ATTRIBUTE_UNUSED)
+default_emutls_object_type (void)
{
- tree word_type_node, field, next_field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__templ),
ptr_type_node);
- DECL_CONTEXT (field) = type;
- next_field = field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__offset),
- ptr_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
- next_field = field;
-
- word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__align),
- word_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
- next_field = field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__size),
word_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
-
- return field;
+ tree word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
+ record_builder rec;
+ rec.add_field (__size, word_type_node);
+ rec.add_field (__align, word_type_node);
+ rec.add_field (__offset, ptr_type_node);
+ rec.add_field (__templ, ptr_type_node);
+ rec.layout ();
That's awkward - you want to hide the fact that layout has
to happen and that it has to happen last. Just make it a
side-effect of .as_tree ().
Sometimes you want to construct recursive types, and for that you
need .as_tree to execute before layout. This feature is used in
the patch.
Note that add_field want's to return the FIELD_DECL created,
people may want to alter it.
Do you have a use case? Until we have one, I'm not convinced that
we should widen the interface.
Note that tag_name does not allow the way C++ uses this (it can
be a TYPE_DECL).
That is what the .decl_name member function does.
Overall I'm not sure this is a good abstraction unless you manage
to make the frontends use it.
The intent is for use by the middle/back ends constructing code.
As such, it should be using middle/back end types, not front-end
types.
--
Lawrence Crowl
Re: [cxx-conversion] Add Record Builder Class
On Tue, Feb 12, 2013 at 2:47 PM, Lawrence Crowl cr...@google.com wrote:
@@ -182,24 +163,9 @@ default_emutls_var_init (tree to, tree d
static tree
get_emutls_object_type (void)
{
- tree type, type_name, field;
-
- type = emutls_object_type;
- if (type)
-return type;
-
- emutls_object_type = type = lang_hooks.types.make_type (RECORD_TYPE);
- type_name = NULL;
- field = targetm.emutls.var_fields (type, type_name);
- if (!type_name)
-type_name = get_identifier (__emutls_object);
- type_name = build_decl (UNKNOWN_LOCATION,
- TYPE_DECL, type_name, type);
- TYPE_NAME (type) = type_name;
- TYPE_FIELDS (type) = field;
- layout_type (type);
-
- return type;
+ if (!emutls_object_type)
+emutls_object_type = targetm.emutls.object_type ();
+ return emutls_object_type;
Hm, this does not look like an idempotent change. Where did I get lost?
===
--- gcc/coverage.c (revision 195904)
+++ gcc/coverage.c (working copy)
@@ -121,8 +121,8 @@ static const char *const ctr_names[GCOV_
/* Forward declarations. */
static void read_counts_file (void);
static tree build_var (tree, tree, int);
-static void build_fn_info_type (tree, unsigned, tree);
-static void build_info_type (tree, tree);
+static tree build_fn_info_type (unsigned, tree);
+static tree build_info_type (record_builder , tree);
I don't really like unnecessary forward declarations. But I guess
it's OK, since you are replacing existing ones.
-static void
-build_fn_info_type (tree type, unsigned counters, tree gcov_info_type)
+static tree
+build_fn_info_type (unsigned counters, tree gcov_info_type)
So, here you are changing the signature because the caller used to
create an empty record and now it doesn't need to? We are going to be
creating it in the constructor, right?
Diego.
Re: [cxx-conversion] Add Record Builder Class
On 2/13/13, Diego Novillo dnovi...@google.com wrote:
On Tue, Feb 12, 2013 at 2:47 PM, Lawrence Crowl cr...@google.com wrote:
@@ -182,24 +163,9 @@ default_emutls_var_init (tree to, tree d
static tree
get_emutls_object_type (void)
{
- tree type, type_name, field;
-
- type = emutls_object_type;
- if (type)
-return type;
-
- emutls_object_type = type = lang_hooks.types.make_type (RECORD_TYPE);
- type_name = NULL;
- field = targetm.emutls.var_fields (type, type_name);
- if (!type_name)
-type_name = get_identifier (__emutls_object);
- type_name = build_decl (UNKNOWN_LOCATION,
- TYPE_DECL, type_name, type);
- TYPE_NAME (type) = type_name;
- TYPE_FIELDS (type) = field;
- layout_type (type);
-
- return type;
+ if (!emutls_object_type)
+emutls_object_type = targetm.emutls.object_type ();
+ return emutls_object_type;
Hm, this does not look like an idempotent change. Where did I
get lost?
The responsibility for creating the type has moved into the new hook.
The old hook only had responsibility for adding fields. And changing
the name if it wasn't right. The new structure has a much clearer
division of responsibility.
===
--- gcc/coverage.c (revision 195904)
+++ gcc/coverage.c (working copy)
@@ -121,8 +121,8 @@ static const char *const ctr_names[GCOV_
/* Forward declarations. */
static void read_counts_file (void);
static tree build_var (tree, tree, int);
-static void build_fn_info_type (tree, unsigned, tree);
-static void build_info_type (tree, tree);
+static tree build_fn_info_type (unsigned, tree);
+static tree build_info_type (record_builder , tree);
I don't really like unnecessary forward declarations. But I guess
it's OK, since you are replacing existing ones.
Yeah. I figure applying style changes should be a separate patch.
-static void
-build_fn_info_type (tree type, unsigned counters, tree gcov_info_type)
+static tree
+build_fn_info_type (unsigned counters, tree gcov_info_type)
So, here you are changing the signature because the caller used
to create an empty record and now it doesn't need to? We are
going to be creating it in the constructor, right?
Correct.
--
Lawrence Crowl
Re: [cxx-conversion] Add Record Builder Class
Thanks. The patch is OK for the branch. You can address Nathan's review after he's back and gets a chance to look at it. Let me know when the patch is in. I've got another merge in process. Diego.
Re: [cxx-conversion] Add Record Builder Class
On 2/13/13, Diego Novillo dnovi...@google.com wrote: Thanks. The patch is OK for the branch. You can address Nathan's review after he's back and gets a chance to look at it. Let me know when the patch is in. I've got another merge in process. Committed. -- Lawrence Crowl
[cxx-conversion] Add Record Builder Class
Add class record_builder to ease construction of records and unions. Use it
in some appropriate places.
Nathan please review the vxworks changes.
tree.h
New class record_builder.
tree.c
Implement record_builder member functions.
asan.c
Change asan_global_struct to use record_builder.
coverage.c
Change build_info_type() to use record_builder. It now takes a
record_builder as a parameter and returns the tree representing
the type.
Change build_fn_info_type() to use record_builder. It now returns
the tree representing the type.
Modify coverage_obj_init() to call them appropriately.
tree-mudflap.c
Change mf_make_mf_cache_struct_type() to use record_builder.
target.def
Replace the emutls var_fields hook with object_type hook. The
essential difference is that the hook is now responsible for full
construction of the type, not just adding fields.
targhooks.h
Replace default_emutls_var_fields() with default_emutls_object_type().
tree-emutls.c
Replace default_emutls_var_fields() with default_emutls_object_type().
Use record_builder within default_emutls_object_type().
Change get_emutls_object_type to use the new target hook.
doc/tm.texi.in
Replace TARGET_EMUTLS_VAR_FIELDS with TARGET_EMUTLS_OBJECT_TYPE.
doc/tm.texi
Replace TARGET_EMUTLS_VAR_FIELDS with TARGET_EMUTLS_OBJECT_TYPE.
config/vxworks.c
Replace vxworks_emutls_var_fields() with vxworks_emutls_object_type().
Use record_builder within vxworks_emutls_object_type().
Tested on x86_64. Tested with config-list.mk on vxworks targets.
Index: gcc/tree-emutls.c
===
--- gcc/tree-emutls.c (revision 195904)
+++ gcc/tree-emutls.c (working copy)
@@ -103,41 +103,22 @@ get_emutls_object_name (tree name)
return prefix_name (prefix, name);
}
-/* Create the fields of the type for the control variables. Ordinarily
+/* Create the type for the control variables. Ordinarily
this must match struct __emutls_object defined in emutls.c. However
this is a target hook so that VxWorks can define its own layout. */
tree
-default_emutls_var_fields (tree type, tree *name ATTRIBUTE_UNUSED)
+default_emutls_object_type (void)
{
- tree word_type_node, field, next_field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__templ), ptr_type_node);
- DECL_CONTEXT (field) = type;
- next_field = field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__offset),
- ptr_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
- next_field = field;
-
- word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__align),
- word_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
- next_field = field;
-
- field = build_decl (UNKNOWN_LOCATION,
- FIELD_DECL, get_identifier (__size), word_type_node);
- DECL_CONTEXT (field) = type;
- DECL_CHAIN (field) = next_field;
-
- return field;
+ tree word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
+ record_builder rec;
+ rec.add_field (__size, word_type_node);
+ rec.add_field (__align, word_type_node);
+ rec.add_field (__offset, ptr_type_node);
+ rec.add_field (__templ, ptr_type_node);
+ rec.layout ();
+ rec.decl_name (__emutls_object);
+ return rec.as_tree ();
}
/* Initialize emulated tls object TO, which refers to TLS variable DECL and
@@ -182,24 +163,9 @@ default_emutls_var_init (tree to, tree d
static tree
get_emutls_object_type (void)
{
- tree type, type_name, field;
-
- type = emutls_object_type;
- if (type)
-return type;
-
- emutls_object_type = type = lang_hooks.types.make_type (RECORD_TYPE);
- type_name = NULL;
- field = targetm.emutls.var_fields (type, type_name);
- if (!type_name)
-type_name = get_identifier (__emutls_object);
- type_name = build_decl (UNKNOWN_LOCATION,
- TYPE_DECL, type_name, type);
- TYPE_NAME (type) = type_name;
- TYPE_FIELDS (type) = field;
- layout_type (type);
-
- return type;
+ if (!emutls_object_type)
+emutls_object_type = targetm.emutls.object_type ();
+ return emutls_object_type;
}
/* Create a read-only variable like DECL, with the same DECL_INITIAL.
Index: gcc/asan.c
===
--- gcc/asan.c (revision 195904)
+++ gcc/asan.c (working copy)
@@ -1496,28 +1496,16 @@ transform_statements (void)
static tree
asan_global_struct (void)
{
- static const char *field_names[5]
-= { __beg, __size, __size_with_redzone,
- __name, __has_dynamic_init };
- tree fields[5], ret;
- int i;
-
- ret = make_node (RECORD_TYPE);
-
