Patchwork Stop gengtype from emitting empty loops

login
register
mail settings
Submitter Laurynas Biveinis
Date July 24, 2012, 3:20 p.m.
Message ID <CAHkCEVeToLuGMWwYP=1kh85_xkimQNK0UvwEBu2t5NuNNSSu4g@mail.gmail.com>
Download mbox | patch
Permalink /patch/172895/
State New
Headers show

Comments

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?
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

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