Patchwork PR54645 move location_adhoc_data map into GC

login
register
mail settings
Submitter Dehao Chen
Date Sept. 21, 2012, 9:07 p.m.
Message ID <CAO2gOZXygjcsWpzem4DNzk_w54u-kLR6H53YiKqazVx797Uw9Q@mail.gmail.com>
Download mbox | patch
Permalink /patch/185932/
State New
Headers show

Comments

Dehao Chen - Sept. 21, 2012, 9:07 p.m.
Hi,

This patch moves location_adhoc_data into GC, and also rebuild the
hash table when reading in the PCH. After the patch, PCH can work as
expected.

Bootstrapped and passed gcc regression tests on x8664_linux.

OK for trunk?

Thanks,
Dehao

libcpp/ChangeLog:
2012-09-21  Dehao Chen  <dehao@google.com>

	PR middle-end/54645
	* include/line-map.h (location_adhoc_data): Move location_adhoc_data
	into GC.
	(location_adhoc_data_map): Likewise.
	(line_maps): Likewise.
	(rebuild_location_adhoc_htab): New Function.
	* line-map.c (+rebuild_location_adhoc_htab): new Funcion.
	(get_combined_adhoc_loc): Move location_adhoc_data into GC.
	(location_adhoc_data_fini): Likewise.
	(linemap_init): Likewise.
	(location_adhoc_data_init): Remove Function.

gcc/ChangeLog:
2012-09-21  Dehao Chen  <dehao@google.com>

	PR middle-end/54645
	* c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data
	map when read in the pch.
Hans-Peter Nilsson - Sept. 24, 2012, 8:23 p.m.
On Fri, 21 Sep 2012, Dehao Chen wrote:
> This patch moves location_adhoc_data into GC, and also rebuild the
> hash table when reading in the PCH. After the patch, PCH can work as
> expected.
>
> Bootstrapped and passed gcc regression tests on x8664_linux.

If you have a moment to consider improvements for the
test-instructions at
<http://gcc.gnu.org/contribute.html#testing> to try and avoid
this situation (introducing regressions), that'd be nice; IIUC
it wasn't clear enough that the "make check" must be at the top
tree?  It seems to say so but maybe that's somehow in a blind
spot.

>
> OK for trunk?
>
> Thanks,
> Dehao
>
> libcpp/ChangeLog:
> 2012-09-21  Dehao Chen  <dehao@google.com>
>
> 	PR middle-end/54645
> 	* include/line-map.h (location_adhoc_data): Move location_adhoc_data
> 	into GC.
> 	(location_adhoc_data_map): Likewise.
> 	(line_maps): Likewise.
> 	(rebuild_location_adhoc_htab): New Function.
> 	* line-map.c (+rebuild_location_adhoc_htab): new Funcion.
> 	(get_combined_adhoc_loc): Move location_adhoc_data into GC.
> 	(location_adhoc_data_fini): Likewise.
> 	(linemap_init): Likewise.
> 	(location_adhoc_data_init): Remove Function.
>
> gcc/ChangeLog:
> 2012-09-21  Dehao Chen  <dehao@google.com>
>
> 	PR middle-end/54645
> 	* c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data
> 	map when read in the pch.


I can't say anything insightful about this patch other than the
nitpick below (i.e. I see nothing wrong with it) but I'd
encourage a proper review of it to resolve the PCH regressions.
There's no PCH section in MAINTAINERS, but "next-of-kin" global
maintainers CC:ed.

I have just a nitpicking remark: please break the overlong
lines.  I noticed the ones with htab_create calls as my viewer
helpfully broke the lines at 80 columns at the last comma.

brgds, H-P
Richard Guenther - Sept. 25, 2012, 7:44 a.m.
On Mon, Sep 24, 2012 at 10:23 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Fri, 21 Sep 2012, Dehao Chen wrote:
>> This patch moves location_adhoc_data into GC, and also rebuild the
>> hash table when reading in the PCH. After the patch, PCH can work as
>> expected.
>>
>> Bootstrapped and passed gcc regression tests on x8664_linux.
>
> If you have a moment to consider improvements for the
> test-instructions at
> <http://gcc.gnu.org/contribute.html#testing> to try and avoid
> this situation (introducing regressions), that'd be nice; IIUC
> it wasn't clear enough that the "make check" must be at the top
> tree?  It seems to say so but maybe that's somehow in a blind
> spot.
>
>>
>> OK for trunk?
>>
>> Thanks,
>> Dehao
>>
>> libcpp/ChangeLog:
>> 2012-09-21  Dehao Chen  <dehao@google.com>
>>
>>       PR middle-end/54645
>>       * include/line-map.h (location_adhoc_data): Move location_adhoc_data
>>       into GC.
>>       (location_adhoc_data_map): Likewise.
>>       (line_maps): Likewise.
>>       (rebuild_location_adhoc_htab): New Function.
>>       * line-map.c (+rebuild_location_adhoc_htab): new Funcion.
>>       (get_combined_adhoc_loc): Move location_adhoc_data into GC.
>>       (location_adhoc_data_fini): Likewise.
>>       (linemap_init): Likewise.
>>       (location_adhoc_data_init): Remove Function.
>>
>> gcc/ChangeLog:
>> 2012-09-21  Dehao Chen  <dehao@google.com>
>>
>>       PR middle-end/54645
>>       * c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data
>>       map when read in the pch.
>
>
> I can't say anything insightful about this patch other than the
> nitpick below (i.e. I see nothing wrong with it) but I'd
> encourage a proper review of it to resolve the PCH regressions.
> There's no PCH section in MAINTAINERS, but "next-of-kin" global
> maintainers CC:ed.

IMHO the PCH implementation is awkward and that is enough of a reason
to remove PCH in its current form.

Richard.

> I have just a nitpicking remark: please break the overlong
> lines.  I noticed the ones with htab_create calls as my viewer
> helpfully broke the lines at 80 columns at the last comma.
>
> brgds, H-P
Eric Botcazou - Sept. 26, 2012, 7:20 a.m.
> gcc/ChangeLog:
> 2012-09-21  Dehao Chen  <dehao@google.com>
> 
> 	PR middle-end/54645
> 	* c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data
> 	map when read in the pch.

Wrong ChangeLog file, you want gcc/c-family/ChangeLog (and remove c-family/).
Dehao Chen - Sept. 26, 2012, 2:50 p.m.
Sorry, I'll change that with my next patch.

Dehao

On Wed, Sep 26, 2012 at 12:20 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> gcc/ChangeLog:
>> 2012-09-21  Dehao Chen  <dehao@google.com>
>>
>>       PR middle-end/54645
>>       * c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data
>>       map when read in the pch.
>
> Wrong ChangeLog file, you want gcc/c-family/ChangeLog (and remove c-family/).
>
> --
> Eric Botcazou

Patch

Index: gcc/c-family/c-pch.c
===================================================================
--- gcc/c-family/c-pch.c	(revision 191618)
+++ gcc/c-family/c-pch.c	(working copy)
@@ -340,6 +340,7 @@  c_common_read_pch (cpp_reader *pfile, const char *
 
   gt_pch_restore (f);
   cpp_set_line_map (pfile, line_table);
+  rebuild_location_adhoc_htab (line_table);
 
   timevar_push (TV_PCH_CPP_RESTORE);
   if (cpp_read_state (pfile, name, f, smd) != 0)
Index: libcpp/include/line-map.h
===================================================================
--- libcpp/include/line-map.h	(revision 191618)
+++ libcpp/include/line-map.h	(working copy)
@@ -260,9 +260,9 @@  struct GTY(()) maps_info {
 };
 
 /* Data structure to associate an arbitrary data to a source location.  */
-struct location_adhoc_data {
+struct GTY(()) location_adhoc_data {
   source_location locus;
-  void *data;
+  void * GTY((skip)) data;
 };
 
 struct htab;
@@ -277,11 +277,11 @@  struct htab;
    bits of the integer is used to index the location_adhoc_data array,
    in which the locus and associated data is stored.  */
 
-struct location_adhoc_data_map {
-  struct htab *htab;
+struct GTY(()) location_adhoc_data_map {
+  struct htab * GTY((skip)) htab;
   source_location curr_loc;
-  struct location_adhoc_data *data;
   unsigned int allocated;
+  struct location_adhoc_data GTY((length ("%h.allocated"))) *data;
 };
 
 /* A set of chronological line_map structures.  */
@@ -315,7 +315,7 @@  struct GTY(()) line_maps {
      allocated, for a certain allocation size requested.  */
   line_map_round_alloc_size_func round_alloc_size;
 
-  struct location_adhoc_data_map GTY((skip)) location_adhoc_data_map;
+  struct location_adhoc_data_map location_adhoc_data_map;
 };
 
 /* Returns the pointer to the memory region where information about
@@ -446,6 +446,8 @@  extern source_location get_location_from_adhoc_loc
 #define COMBINE_LOCATION_DATA(SET, LOC, BLOCK) \
   get_combined_adhoc_loc ((SET), (LOC), (BLOCK))
 
+extern void rebuild_location_adhoc_htab (struct line_maps *);
+
 /* Initialize a line map set.  */
 extern void linemap_init (struct line_maps *);
 
Index: libcpp/line-map.c
===================================================================
--- libcpp/line-map.c	(revision 191618)
+++ libcpp/line-map.c	(working copy)
@@ -82,6 +82,19 @@  location_adhoc_data_update (void **slot, void *dat
   return 1;
 }
 
+/* Rebuild the hash table from the location adhoc data.  */
+
+void
+rebuild_location_adhoc_htab (struct line_maps *set)
+{
+  unsigned i;
+  set->location_adhoc_data_map.htab =
+      htab_create (100, location_adhoc_data_hash, location_adhoc_data_eq, NULL);
+  for (i = 0; i < set->location_adhoc_data_map.curr_loc; i++)
+    htab_find_slot (set->location_adhoc_data_map.htab,
+		    set->location_adhoc_data_map.data + i, INSERT);
+}
+
 /* Combine LOCUS and DATA to a combined adhoc loc.  */
 
 source_location
@@ -109,14 +122,21 @@  get_combined_adhoc_loc (struct line_maps *set,
 	{
 	  char *orig_data = (char *) set->location_adhoc_data_map.data;
 	  long long offset;
-	  set->location_adhoc_data_map.allocated *= 2;
-	  set->location_adhoc_data_map.data =
-	      XRESIZEVEC (struct location_adhoc_data,
-			  set->location_adhoc_data_map.data,
-			  set->location_adhoc_data_map.allocated);
+	  line_map_realloc reallocator
+	      = set->reallocator ? set->reallocator : xrealloc;
+
+	  if (set->location_adhoc_data_map.allocated == 0)
+	    set->location_adhoc_data_map.allocated = 128;
+	  else
+	    set->location_adhoc_data_map.allocated *= 2;
+	  set->location_adhoc_data_map.data = (struct location_adhoc_data *)
+	      reallocator (set->location_adhoc_data_map.data,
+			   set->location_adhoc_data_map.allocated
+			   * sizeof (struct location_adhoc_data));
 	  offset = (char *) (set->location_adhoc_data_map.data) - orig_data;
-	  htab_traverse (set->location_adhoc_data_map.htab,
-			 location_adhoc_data_update, &offset);
+	  if (set->location_adhoc_data_map.allocated > 128)
+	    htab_traverse (set->location_adhoc_data_map.htab,
+			   location_adhoc_data_update, &offset);
 	}
       *slot = set->location_adhoc_data_map.data
 	      + set->location_adhoc_data_map.curr_loc;
@@ -144,24 +164,10 @@  get_location_from_adhoc_loc (struct line_maps *set
   return set->location_adhoc_data_map.data[loc & MAX_SOURCE_LOCATION].locus;
 }
 
-/* Initialize the location_adhoc_data structure.  */
-
-static void
-location_adhoc_data_init (struct line_maps *set)
-{
-  set->location_adhoc_data_map.htab =
-      htab_create (100, location_adhoc_data_hash, location_adhoc_data_eq, NULL);
-  set->location_adhoc_data_map.curr_loc = 0;
-  set->location_adhoc_data_map.allocated = 100;
-  set->location_adhoc_data_map.data = XNEWVEC (struct location_adhoc_data, 100);
-}
-
 /* Finalize the location_adhoc_data structure.  */
 void
 location_adhoc_data_fini (struct line_maps *set)
 {
-  set->location_adhoc_data_map.allocated = 0;
-  XDELETEVEC (set->location_adhoc_data_map.data);
   htab_delete (set->location_adhoc_data_map.htab);
 }
 
@@ -173,7 +179,8 @@  linemap_init (struct line_maps *set)
   memset (set, 0, sizeof (struct line_maps));
   set->highest_location = RESERVED_LOCATION_COUNT - 1;
   set->highest_line = RESERVED_LOCATION_COUNT - 1;
-  location_adhoc_data_init (set);
+  set->location_adhoc_data_map.htab =
+      htab_create (100, location_adhoc_data_hash, location_adhoc_data_eq, NULL);
 }
 
 /* Check for and warn about line_maps entered but not exited.  */