Stop gengtype from emitting empty loops

Submitted by Laurynas Biveinis on July 24, 2012, 3:20 p.m.

Details

Message ID CAHkCEVeToLuGMWwYP=1kh85_xkimQNK0UvwEBu2t5NuNNSSu4g@mail.gmail.com
State New
Headers show

Commit Message

Laurynas Biveinis July 24, 2012, 3:20 p.m.
>>> I think it merely points to a bogus GTY annotation, not sure if we want to
>>> gobble this kind of gengtype hacks which only benefit -O0 ...

> ../../trunk/gcc/../libcpp/include/line-map.h:168: option `length' may
> not be applied to arrays of scalar types
> ../../trunk/gcc/emit-rtl.c:5913: option `length' may not be applied to
> arrays of scalar types
> ../../trunk/gcc/java/jcf.h:85: option `length' may not be applied to
> arrays of scalar types
> ../../trunk/gcc/java/jcf.h:94: option `length' may not be applied to
> arrays of scalar types

Below is the patch that fixes all these. A gtype-desc.c diff shows a
number of empty loops removed. Regarding vector, I didn't see any
other way to fix it (except ignoring it) but invent a new type of
vector for atomic types.

> Index: gengtype.c
> ===================================================================
> --- gengtype.c  (revision 189778)
> +++ gengtype.c  (working copy)
> @@ -1256,7 +1256,17 @@ adjust_field_type (type_p t, options_p opt)
>
>    for (; opt; opt = opt->next)
>      if (strcmp (opt->name, "length") == 0)
> -      length_p = 1;
> +      {
> +       if (length_p)
> +         error_at_line (&lexer_line, "duplicate `%s' option", opt->name);
> +       if (t->u.p->kind == TYPE_SCALAR && ! t->u.p->u.scalar_is_char)
> +         {
> +           error_at_line (&lexer_line,
> +                          "option `%s' may not be applied to arrays
> of scalar types",
> +                          opt->name);
> +         }
> +       length_p = 1;
> +      }
>      else if ((strcmp (opt->name, "param_is") == 0
>               || (strncmp (opt->name, "param", 5) == 0
>                   && ISDIGIT (opt->name[5])

s/arrays of scalar types/arrays of atomic types, add a ChangeLog entry
and the snippet above is approved ;)


libcpp:
2012-06-24  Laurynas Bivienis  <laurynas.biveinis@gmail.com>

	* include/line-map.h (line_map_macro): Use the "atomic" GTY option
	for the macro_locations field.

gcc/java:
2012-07-24  Laurynas Biveinis  <laurynas.biveinis@gmail.com>

	* jcf.h (CPool): Use the "atomic" GTY option for the tags field.
	(bootstrap_method): Likewise for the bootstrap_arguments field.

gcc:
2012-07-24  Laurynas Biveinis  <laurynas.biveinis@gmail.com>

	* doc/gty.texi: Clarify that GTY option "length" is only for
	arrays of non-atomic objects.  Fix typo in the description of the
	"atomic" option.
	* vec.h: Describe the atomic object "A" type of the macros in
	the header comment.
	(VEC_T_GTY_ATOMIC, DEF_VEC_A, DEF_VEC_ALLOC_A): Define.
	* emit-rtl.c (locations_locators_vals): use the atomic object
	vector.

OK for trunk if it passes testing?

Comments

Steven Bosscher July 24, 2012, 3:23 p.m.
On Tue, Jul 24, 2012 at 5:20 PM, Laurynas Biveinis
<laurynas.biveinis@gmail.com> wrote:
> s/arrays of scalar types/arrays of atomic types, add a ChangeLog entry
> and the snippet above is approved ;)

Can you commit it along with your fixes? I think it'd be good to keep
this and your fixes together in a single commit.

Thanks for working on this,

Ciao!
Steven
Tom Tromey July 24, 2012, 7:11 p.m.
>>>>> "Laurynas" == Laurynas Biveinis <laurynas.biveinis@gmail.com> writes:

Laurynas> 2012-06-24  Laurynas Bivienis  <laurynas.biveinis@gmail.com>
Laurynas> 	* include/line-map.h (line_map_macro): Use the "atomic" GTY option
Laurynas> 	for the macro_locations field.

Laurynas> gcc/java:
Laurynas> 2012-07-24  Laurynas Biveinis  <laurynas.biveinis@gmail.com>
Laurynas> 	* jcf.h (CPool): Use the "atomic" GTY option for the tags field.
Laurynas> 	(bootstrap_method): Likewise for the bootstrap_arguments field.

These parts are ok.

Tom
Laurynas Biveinis July 25, 2012, 3:34 a.m.
>> s/arrays of scalar types/arrays of atomic types, add a ChangeLog entry
>> and the snippet above is approved ;)
>
> Can you commit it along with your fixes? I think it'd be good to keep
> this and your fixes together in a single commit.

OK

Patch hide | download patch | download mbox

Index: libcpp/include/line-map.h
===================================================================
--- libcpp/include/line-map.h	(revision 189802)
+++ libcpp/include/line-map.h	(working copy)
@@ -165,7 +165,7 @@ 
      In the example above x1 (for token "+") is going to be the same
      as y1.  x0 is the spelling location for the argument token "1",
      and x2 is the spelling location for the argument token "2".  */
-  source_location * GTY((length ("2 * %h.n_tokens"))) macro_locations;
+  source_location * GTY((atomic)) macro_locations;
 
   /* This is the location of the expansion point of the current macro
      map.  It's the location of the macro name.  That location is held
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	(revision 189802)
+++ gcc/emit-rtl.c	(working copy)
@@ -5910,8 +5910,8 @@ 
 static VEC(int,heap) *block_locators_locs;
 static GTY(()) VEC(tree,gc) *block_locators_blocks;
 static VEC(int,heap) *locations_locators_locs;
-DEF_VEC_O(location_t);
-DEF_VEC_ALLOC_O(location_t,heap);
+DEF_VEC_A(location_t);
+DEF_VEC_ALLOC_A(location_t,heap);
 static VEC(location_t,heap) *locations_locators_vals;
 int prologue_locator;
 int epilogue_locator;
Index: gcc/vec.h
===================================================================
--- gcc/vec.h	(revision 189802)
+++ gcc/vec.h	(working copy)
@@ -95,24 +95,25 @@ 
    the 'space' predicate will tell you whether there is spare capacity
    in the vector.  You will not normally need to use these two functions.
 
-   Vector types are defined using a DEF_VEC_{O,P,I}(TYPEDEF) macro, to
+   Vector types are defined using a DEF_VEC_{O,A,P,I}(TYPEDEF) macro, to
    get the non-memory allocation version, and then a
-   DEF_VEC_ALLOC_{O,P,I}(TYPEDEF,ALLOC) macro to get memory managed
+   DEF_VEC_ALLOC_{O,A,P,I}(TYPEDEF,ALLOC) macro to get memory managed
    vectors.  Variables of vector type are declared using a
    VEC(TYPEDEF,ALLOC) macro.  The ALLOC argument specifies the
    allocation strategy, and can be either 'gc' or 'heap' for garbage
    collected and heap allocated respectively.  It can be 'none' to get
    a vector that must be explicitly allocated (for instance as a
-   trailing array of another structure).  The characters O, P and I
-   indicate whether TYPEDEF is a pointer (P), object (O) or integral
-   (I) type.  Be careful to pick the correct one, as you'll get an
-   awkward and inefficient API if you use the wrong one.  There is a
-   check, which results in a compile-time warning, for the P and I
-   versions, but there is no check for the O versions, as that is not
-   possible in plain C.  Due to the way GTY works, you must annotate
-   any structures you wish to insert or reference from a vector with a
-   GTY(()) tag.  You need to do this even if you never declare the GC
-   allocated variants.
+   trailing array of another structure).  The characters O, A, P and I
+   indicate whether TYPEDEF is a pointer (P), object (O), atomic object
+   (A) or integral (I) type.  Be careful to pick the correct one, as
+   you'll get an awkward and inefficient API if you use the wrong one or
+   a even a crash if you pick the atomic object version when the object
+   version should have been chosen instead.  There is a check, which
+   results in a compile-time warning, for the P and I versions, but there
+   is no check for the O versions, as that is not possible in plain C.
+   Due to the way GTY works, you must annotate any structures you wish to
+   insert or reference from a vector with a GTY(()) tag.  You need to do
+   this even if you never declare the GC allocated variants.
 
    An example of their use would be,
 
@@ -535,6 +536,13 @@ 
   T GTY ((length ("%h.prefix.num"))) vec[1];				  \
 } VEC(T,B)
 
+#define VEC_T_GTY_ATOMIC(T,B)						  \
+typedef struct GTY(()) VEC(T,B)						  \
+{									  \
+  struct vec_prefix prefix;						  \
+  T GTY ((atomic)) vec[1];						  \
+} VEC(T,B)
+
 /* Derived vector type, user visible.  */
 #define VEC_TA_GTY(T,B,A,GTY)						  \
 typedef struct GTY VEC(T,A)						  \
@@ -909,6 +917,14 @@ 
 DEF_VEC_NONALLOC_FUNCS_O(T,A)						  \
 struct vec_swallow_trailing_semi
 
+/* Vector of atomic object.  */
+#define DEF_VEC_A(T)							  \
+VEC_T_GTY_ATOMIC(T,base);						  \
+VEC_TA(T,base,none);							  \
+DEF_VEC_FUNC_O(T)							  \
+struct vec_swallow_trailing_semi
+#define DEF_VEC_ALLOC_A(T,A) DEF_VEC_ALLOC_O(T,A)
+
 #define DEF_VEC_FUNC_O(T)						  \
 static inline unsigned VEC_OP (T,base,length) (const VEC(T,base) *vec_)	  \
 {									  \
Index: gcc/java/jcf.h
===================================================================
--- gcc/java/jcf.h	(revision 189802)
+++ gcc/java/jcf.h	(working copy)
@@ -82,7 +82,7 @@ 
   /* The constant_pool_count. */
   int		count;
 
-  uint8* GTY((length ("%h.count")))	tags;
+  uint8 * GTY((atomic)) tags;
 
   union cpool_entry * GTY((length ("%h.count"),
 			   desc ("cpool_entry_is_tree (%1.tags%a)")))	data;
@@ -91,7 +91,7 @@ 
 typedef struct GTY(()) bootstrap_method {
   unsigned method_ref;
   unsigned num_arguments;
-  unsigned* GTY((length ("%h.num_arguments"))) bootstrap_arguments;
+  unsigned * GTY((atomic)) bootstrap_arguments;
 } bootstrap_method;
 
 typedef struct GTY(()) BootstrapMethods {
Index: gcc/doc/gty.texi
===================================================================
--- gcc/doc/gty.texi	(revision 189802)
+++ gcc/doc/gty.texi	(working copy)
@@ -134,8 +134,8 @@ 
 @item length ("@var{expression}")
 
 There are two places the type machinery will need to be explicitly told
-the length of an array.  The first case is when a structure ends in a
-variable-length array, like this:
+the length of an array of non-atomic objects.  The first case is when a
+structure ends in a variable-length array, like this:
 @smallexample
 struct GTY(()) rtvec_def @{
   int num_elem;         /* @r{number of elements} */
@@ -163,6 +163,11 @@ 
 static GTY((length("reg_known_value_size"))) rtx *reg_known_value;
 @end verbatim
 
+Note that the @code{length} option is only meant for use with arrays of
+non-atomic objects, that is, objects that contain pointers pointing to
+other GTY-managed objects.  For other arrays you should use @code{atomic}
+or even @code{skip}.
+
 @findex skip
 @item skip
 
@@ -411,7 +416,7 @@ 
 @smallexample
 struct GTY(()) my_struct @{
   int number_of_elements;
-  unsigned int GTY ((atomic)) * elements;
+  unsigned int * GTY ((atomic)) elements;
 @};
 @end smallexample
 In this case, @code{elements} is a pointer under GC, and the memory it