* Turn streamer cache to pointer_map
@ 2011-05-02 14:16 Jan Hubicka
2011-05-02 14:35 ` Michael Matz
2011-05-02 14:39 ` Richard Guenther
0 siblings, 2 replies; 12+ messages in thread
From: Jan Hubicka @ 2011-05-02 14:16 UTC (permalink / raw)
To: gcc-patches
Hi,
according to oprofile, libiberty hashing takes about 30% of streaming in time
and according to callgrind, the most busy cache is node_map cache in the
streamer.
This patch turns it into pointer-map that also saves about 400MB of memory
and is bit prettier. I get about 8-10% speedup on Mozilla streaming.
There are other uses of tree_int map, I could probably convert them, too.
Bootstrapped/regtested x86_64-linux, OK?
Honza
* lto-streamer.c (lto_streamer_cache_insert_1,
lto_streamer_cache_lookup, lto_streamer_cache_create,
lto_streamer_cache_delete): Use pointer map instead of hashtable.
* lto-streamer.h (lto_streamer_cache_d): Turn node_map into pointer_map.
Index: lto-streamer.c
===================================================================
*** lto-streamer.c (revision 173251)
--- lto-streamer.c (working copy)
*************** lto_streamer_cache_insert_1 (struct lto_
*** 348,373 ****
bool insert_at_next_slot_p)
{
void **slot;
- struct tree_int_map d_entry, *entry;
unsigned ix;
bool existed_p;
gcc_assert (t);
! d_entry.base.from = t;
! slot = htab_find_slot (cache->node_map, &d_entry, INSERT);
! if (*slot == NULL)
{
/* Determine the next slot to use in the cache. */
if (insert_at_next_slot_p)
ix = VEC_length (tree, cache->nodes);
else
ix = *ix_p;
!
! entry = (struct tree_int_map *)pool_alloc (cache->node_map_entries);
! entry->base.from = t;
! entry->to = ix;
! *slot = entry;
lto_streamer_cache_add_to_node_array (cache, ix, t);
--- 348,367 ----
bool insert_at_next_slot_p)
{
void **slot;
unsigned ix;
bool existed_p;
gcc_assert (t);
! slot = pointer_map_insert (cache->node_map, t);
! if (!*slot)
{
/* Determine the next slot to use in the cache. */
if (insert_at_next_slot_p)
ix = VEC_length (tree, cache->nodes);
else
ix = *ix_p;
! *slot = (void *)(size_t) (ix);
lto_streamer_cache_add_to_node_array (cache, ix, t);
*************** lto_streamer_cache_insert_1 (struct lto_
*** 376,383 ****
}
else
{
! entry = (struct tree_int_map *) *slot;
! ix = entry->to;
if (!insert_at_next_slot_p && ix != *ix_p)
{
--- 370,376 ----
}
else
{
! ix = (size_t) *slot;
if (!insert_at_next_slot_p && ix != *ix_p)
{
*************** lto_streamer_cache_lookup (struct lto_st
*** 442,455 ****
unsigned *ix_p)
{
void **slot;
- struct tree_int_map d_slot;
bool retval;
unsigned ix;
gcc_assert (t);
! d_slot.base.from = t;
! slot = htab_find_slot (cache->node_map, &d_slot, NO_INSERT);
if (slot == NULL)
{
retval = false;
--- 435,446 ----
unsigned *ix_p)
{
void **slot;
bool retval;
unsigned ix;
gcc_assert (t);
! slot = pointer_map_contains (cache->node_map, t);
if (slot == NULL)
{
retval = false;
*************** lto_streamer_cache_lookup (struct lto_st
*** 458,464 ****
else
{
retval = true;
! ix = ((struct tree_int_map *) *slot)->to;
}
if (ix_p)
--- 449,455 ----
else
{
retval = true;
! ix = (size_t) *slot;
}
if (ix_p)
*************** lto_streamer_cache_create (void)
*** 608,618 ****
cache = XCNEW (struct lto_streamer_cache_d);
! cache->node_map = htab_create (101, tree_int_map_hash, tree_int_map_eq, NULL);
!
! cache->node_map_entries = create_alloc_pool ("node map",
! sizeof (struct tree_int_map),
! 100);
/* Load all the well-known tree nodes that are always created by
the compiler on startup. This prevents writing them out
--- 599,605 ----
cache = XCNEW (struct lto_streamer_cache_d);
! cache->node_map = pointer_map_create ();
/* Load all the well-known tree nodes that are always created by
the compiler on startup. This prevents writing them out
*************** lto_streamer_cache_delete (struct lto_st
*** 636,643 ****
if (c == NULL)
return;
! htab_delete (c->node_map);
! free_alloc_pool (c->node_map_entries);
VEC_free (tree, heap, c->nodes);
free (c);
}
--- 623,629 ----
if (c == NULL)
return;
! pointer_map_destroy (c->node_map);
VEC_free (tree, heap, c->nodes);
free (c);
}
Index: lto-streamer.h
===================================================================
*** lto-streamer.h (revision 173251)
--- lto-streamer.h (working copy)
*************** typedef void (lto_free_section_data_f) (
*** 346,355 ****
struct lto_streamer_cache_d
{
/* The mapping between tree nodes and slots into the nodes array. */
! htab_t node_map;
!
! /* Node map to store entries into. */
! alloc_pool node_map_entries;
/* The nodes pickled so far. */
VEC(tree,heap) *nodes;
--- 346,352 ----
struct lto_streamer_cache_d
{
/* The mapping between tree nodes and slots into the nodes array. */
! struct pointer_map_t GTY((skip)) *node_map;
/* The nodes pickled so far. */
VEC(tree,heap) *nodes;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 14:16 Turn streamer cache to pointer_map Jan Hubicka
@ 2011-05-02 14:35 ` Michael Matz
2011-05-02 15:14 ` Richard Guenther
2011-05-02 14:39 ` Richard Guenther
1 sibling, 1 reply; 12+ messages in thread
From: Michael Matz @ 2011-05-02 14:35 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches
Hi,
On Mon, 2 May 2011, Jan Hubicka wrote:
> ! d_entry.base.from = t;
> ! slot = htab_find_slot (cache->node_map, &d_entry, INSERT);
> ! if (*slot == NULL)
> {
> /* Determine the next slot to use in the cache. */
> if (insert_at_next_slot_p)
> ix = VEC_length (tree, cache->nodes);
> else
> ix = *ix_p;
> !
> ! entry = (struct tree_int_map *)pool_alloc (cache->node_map_entries);
> ! entry->base.from = t;
> ! entry->to = ix;
> ! *slot = entry;
>
> lto_streamer_cache_add_to_node_array (cache, ix, t);
>
> --- 348,367 ----
> bool insert_at_next_slot_p)
> {
> void **slot;
> unsigned ix;
> bool existed_p;
>
> gcc_assert (t);
>
> ! slot = pointer_map_insert (cache->node_map, t);
> ! if (!*slot)
ix might legitimately be zero. Hence this transformation is not
equivalent. You might want to enter ix+1 into the cache with the
appropriate adjustment at read-out. Same for the other places.
Ciao,
Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 14:16 Turn streamer cache to pointer_map Jan Hubicka
2011-05-02 14:35 ` Michael Matz
@ 2011-05-02 14:39 ` Richard Guenther
2011-05-02 14:46 ` Richard Guenther
2011-05-02 14:57 ` Jan Hubicka
1 sibling, 2 replies; 12+ messages in thread
From: Richard Guenther @ 2011-05-02 14:39 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches
On Mon, May 2, 2011 at 4:16 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> according to oprofile, libiberty hashing takes about 30% of streaming in time
> and according to callgrind, the most busy cache is node_map cache in the
> streamer.
>
> This patch turns it into pointer-map that also saves about 400MB of memory
> and is bit prettier. I get about 8-10% speedup on Mozilla streaming.
> There are other uses of tree_int map, I could probably convert them, too.
>
> Bootstrapped/regtested x86_64-linux, OK?
>
> Honza
>
> * lto-streamer.c (lto_streamer_cache_insert_1,
> lto_streamer_cache_lookup, lto_streamer_cache_create,
> lto_streamer_cache_delete): Use pointer map instead of hashtable.
> * lto-streamer.h (lto_streamer_cache_d): Turn node_map into pointer_map.
> Index: lto-streamer.c
> ===================================================================
> *** lto-streamer.c (revision 173251)
> --- lto-streamer.c (working copy)
> *************** lto_streamer_cache_insert_1 (struct lto_
> *** 348,373 ****
> bool insert_at_next_slot_p)
> {
> void **slot;
> - struct tree_int_map d_entry, *entry;
> unsigned ix;
> bool existed_p;
>
> gcc_assert (t);
>
> ! d_entry.base.from = t;
> ! slot = htab_find_slot (cache->node_map, &d_entry, INSERT);
> ! if (*slot == NULL)
> {
> /* Determine the next slot to use in the cache. */
> if (insert_at_next_slot_p)
> ix = VEC_length (tree, cache->nodes);
> else
> ix = *ix_p;
> !
> ! entry = (struct tree_int_map *)pool_alloc (cache->node_map_entries);
> ! entry->base.from = t;
> ! entry->to = ix;
> ! *slot = entry;
>
> lto_streamer_cache_add_to_node_array (cache, ix, t);
>
> --- 348,367 ----
> bool insert_at_next_slot_p)
> {
> void **slot;
> unsigned ix;
> bool existed_p;
>
> gcc_assert (t);
>
> ! slot = pointer_map_insert (cache->node_map, t);
> ! if (!*slot)
> {
> /* Determine the next slot to use in the cache. */
> if (insert_at_next_slot_p)
> ix = VEC_length (tree, cache->nodes);
> else
> ix = *ix_p;
> ! *slot = (void *)(size_t) (ix);
>
> lto_streamer_cache_add_to_node_array (cache, ix, t);
>
> *************** lto_streamer_cache_insert_1 (struct lto_
> *** 376,383 ****
> }
> else
> {
> ! entry = (struct tree_int_map *) *slot;
> ! ix = entry->to;
>
> if (!insert_at_next_slot_p && ix != *ix_p)
> {
> --- 370,376 ----
> }
> else
> {
> ! ix = (size_t) *slot;
>
> if (!insert_at_next_slot_p && ix != *ix_p)
> {
> *************** lto_streamer_cache_lookup (struct lto_st
> *** 442,455 ****
> unsigned *ix_p)
> {
> void **slot;
> - struct tree_int_map d_slot;
> bool retval;
> unsigned ix;
>
> gcc_assert (t);
>
> ! d_slot.base.from = t;
> ! slot = htab_find_slot (cache->node_map, &d_slot, NO_INSERT);
> if (slot == NULL)
> {
> retval = false;
> --- 435,446 ----
> unsigned *ix_p)
> {
> void **slot;
> bool retval;
> unsigned ix;
>
> gcc_assert (t);
>
> ! slot = pointer_map_contains (cache->node_map, t);
> if (slot == NULL)
> {
> retval = false;
> *************** lto_streamer_cache_lookup (struct lto_st
> *** 458,464 ****
> else
> {
> retval = true;
> ! ix = ((struct tree_int_map *) *slot)->to;
> }
>
> if (ix_p)
> --- 449,455 ----
> else
> {
> retval = true;
> ! ix = (size_t) *slot;
> }
>
> if (ix_p)
> *************** lto_streamer_cache_create (void)
> *** 608,618 ****
>
> cache = XCNEW (struct lto_streamer_cache_d);
>
> ! cache->node_map = htab_create (101, tree_int_map_hash, tree_int_map_eq, NULL);
> !
> ! cache->node_map_entries = create_alloc_pool ("node map",
> ! sizeof (struct tree_int_map),
> ! 100);
>
> /* Load all the well-known tree nodes that are always created by
> the compiler on startup. This prevents writing them out
> --- 599,605 ----
>
> cache = XCNEW (struct lto_streamer_cache_d);
>
> ! cache->node_map = pointer_map_create ();
>
> /* Load all the well-known tree nodes that are always created by
> the compiler on startup. This prevents writing them out
> *************** lto_streamer_cache_delete (struct lto_st
> *** 636,643 ****
> if (c == NULL)
> return;
>
> ! htab_delete (c->node_map);
> ! free_alloc_pool (c->node_map_entries);
> VEC_free (tree, heap, c->nodes);
> free (c);
> }
> --- 623,629 ----
> if (c == NULL)
> return;
>
> ! pointer_map_destroy (c->node_map);
> VEC_free (tree, heap, c->nodes);
> free (c);
> }
> Index: lto-streamer.h
> ===================================================================
> *** lto-streamer.h (revision 173251)
> --- lto-streamer.h (working copy)
> *************** typedef void (lto_free_section_data_f) (
> *** 346,355 ****
> struct lto_streamer_cache_d
> {
> /* The mapping between tree nodes and slots into the nodes array. */
> ! htab_t node_map;
> !
> ! /* Node map to store entries into. */
> ! alloc_pool node_map_entries;
>
> /* The nodes pickled so far. */
> VEC(tree,heap) *nodes;
> --- 346,352 ----
> struct lto_streamer_cache_d
> {
> /* The mapping between tree nodes and slots into the nodes array. */
> ! struct pointer_map_t GTY((skip)) *node_map;
If you skip node_map you can end up with false entries for re-used
trees. So I don't think that's a good idea.
Richard.
>
> /* The nodes pickled so far. */
> VEC(tree,heap) *nodes;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 14:39 ` Richard Guenther
@ 2011-05-02 14:46 ` Richard Guenther
2011-05-02 14:49 ` Nathan Froyd
2011-05-02 14:52 ` Michael Matz
2011-05-02 14:57 ` Jan Hubicka
1 sibling, 2 replies; 12+ messages in thread
From: Richard Guenther @ 2011-05-02 14:46 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches
On Mon, May 2, 2011 at 4:38 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, May 2, 2011 at 4:16 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi,
>> according to oprofile, libiberty hashing takes about 30% of streaming in time
>> and according to callgrind, the most busy cache is node_map cache in the
>> streamer.
>>
>> This patch turns it into pointer-map that also saves about 400MB of memory
>> and is bit prettier. I get about 8-10% speedup on Mozilla streaming.
>> There are other uses of tree_int map, I could probably convert them, too.
>>
>> Bootstrapped/regtested x86_64-linux, OK?
>>
>> Honza
>>
>> * lto-streamer.c (lto_streamer_cache_insert_1,
>> lto_streamer_cache_lookup, lto_streamer_cache_create,
>> lto_streamer_cache_delete): Use pointer map instead of hashtable.
>> * lto-streamer.h (lto_streamer_cache_d): Turn node_map into pointer_map.
>> Index: lto-streamer.c
>> ===================================================================
>> *** lto-streamer.c (revision 173251)
>> --- lto-streamer.c (working copy)
>> *************** lto_streamer_cache_insert_1 (struct lto_
>> *** 348,373 ****
>> bool insert_at_next_slot_p)
>> {
>> void **slot;
>> - struct tree_int_map d_entry, *entry;
>> unsigned ix;
>> bool existed_p;
>>
>> gcc_assert (t);
>>
>> ! d_entry.base.from = t;
>> ! slot = htab_find_slot (cache->node_map, &d_entry, INSERT);
>> ! if (*slot == NULL)
>> {
>> /* Determine the next slot to use in the cache. */
>> if (insert_at_next_slot_p)
>> ix = VEC_length (tree, cache->nodes);
>> else
>> ix = *ix_p;
>> !
>> ! entry = (struct tree_int_map *)pool_alloc (cache->node_map_entries);
>> ! entry->base.from = t;
>> ! entry->to = ix;
>> ! *slot = entry;
>>
>> lto_streamer_cache_add_to_node_array (cache, ix, t);
>>
>> --- 348,367 ----
>> bool insert_at_next_slot_p)
>> {
>> void **slot;
>> unsigned ix;
>> bool existed_p;
>>
>> gcc_assert (t);
>>
>> ! slot = pointer_map_insert (cache->node_map, t);
>> ! if (!*slot)
>> {
>> /* Determine the next slot to use in the cache. */
>> if (insert_at_next_slot_p)
>> ix = VEC_length (tree, cache->nodes);
>> else
>> ix = *ix_p;
>> ! *slot = (void *)(size_t) (ix);
>>
>> lto_streamer_cache_add_to_node_array (cache, ix, t);
>>
>> *************** lto_streamer_cache_insert_1 (struct lto_
>> *** 376,383 ****
>> }
>> else
>> {
>> ! entry = (struct tree_int_map *) *slot;
>> ! ix = entry->to;
>>
>> if (!insert_at_next_slot_p && ix != *ix_p)
>> {
>> --- 370,376 ----
>> }
>> else
>> {
>> ! ix = (size_t) *slot;
>>
>> if (!insert_at_next_slot_p && ix != *ix_p)
>> {
>> *************** lto_streamer_cache_lookup (struct lto_st
>> *** 442,455 ****
>> unsigned *ix_p)
>> {
>> void **slot;
>> - struct tree_int_map d_slot;
>> bool retval;
>> unsigned ix;
>>
>> gcc_assert (t);
>>
>> ! d_slot.base.from = t;
>> ! slot = htab_find_slot (cache->node_map, &d_slot, NO_INSERT);
>> if (slot == NULL)
>> {
>> retval = false;
>> --- 435,446 ----
>> unsigned *ix_p)
>> {
>> void **slot;
>> bool retval;
>> unsigned ix;
>>
>> gcc_assert (t);
>>
>> ! slot = pointer_map_contains (cache->node_map, t);
>> if (slot == NULL)
>> {
>> retval = false;
>> *************** lto_streamer_cache_lookup (struct lto_st
>> *** 458,464 ****
>> else
>> {
>> retval = true;
>> ! ix = ((struct tree_int_map *) *slot)->to;
>> }
>>
>> if (ix_p)
>> --- 449,455 ----
>> else
>> {
>> retval = true;
>> ! ix = (size_t) *slot;
>> }
>>
>> if (ix_p)
>> *************** lto_streamer_cache_create (void)
>> *** 608,618 ****
>>
>> cache = XCNEW (struct lto_streamer_cache_d);
>>
>> ! cache->node_map = htab_create (101, tree_int_map_hash, tree_int_map_eq, NULL);
>> !
>> ! cache->node_map_entries = create_alloc_pool ("node map",
>> ! sizeof (struct tree_int_map),
>> ! 100);
>>
>> /* Load all the well-known tree nodes that are always created by
>> the compiler on startup. This prevents writing them out
>> --- 599,605 ----
>>
>> cache = XCNEW (struct lto_streamer_cache_d);
>>
>> ! cache->node_map = pointer_map_create ();
>>
>> /* Load all the well-known tree nodes that are always created by
>> the compiler on startup. This prevents writing them out
>> *************** lto_streamer_cache_delete (struct lto_st
>> *** 636,643 ****
>> if (c == NULL)
>> return;
>>
>> ! htab_delete (c->node_map);
>> ! free_alloc_pool (c->node_map_entries);
>> VEC_free (tree, heap, c->nodes);
>> free (c);
>> }
>> --- 623,629 ----
>> if (c == NULL)
>> return;
>>
>> ! pointer_map_destroy (c->node_map);
>> VEC_free (tree, heap, c->nodes);
>> free (c);
>> }
>> Index: lto-streamer.h
>> ===================================================================
>> *** lto-streamer.h (revision 173251)
>> --- lto-streamer.h (working copy)
>> *************** typedef void (lto_free_section_data_f) (
>> *** 346,355 ****
>> struct lto_streamer_cache_d
>> {
>> /* The mapping between tree nodes and slots into the nodes array. */
>> ! htab_t node_map;
>> !
>> ! /* Node map to store entries into. */
>> ! alloc_pool node_map_entries;
>>
>> /* The nodes pickled so far. */
>> VEC(tree,heap) *nodes;
>> --- 346,352 ----
>> struct lto_streamer_cache_d
>> {
>> /* The mapping between tree nodes and slots into the nodes array. */
>> ! struct pointer_map_t GTY((skip)) *node_map;
>
> If you skip node_map you can end up with false entries for re-used
> trees. So I don't think that's a good idea.
Or we can safely mark the whole struct as non-GC (and also allocate
it that way).
Richard.
> Richard.
>
>>
>> /* The nodes pickled so far. */
>> VEC(tree,heap) *nodes;
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 14:46 ` Richard Guenther
@ 2011-05-02 14:49 ` Nathan Froyd
2011-05-02 14:52 ` Michael Matz
1 sibling, 0 replies; 12+ messages in thread
From: Nathan Froyd @ 2011-05-02 14:49 UTC (permalink / raw)
To: Richard Guenther; +Cc: Jan Hubicka, gcc-patches
On Mon, May 02, 2011 at 04:46:23PM +0200, Richard Guenther wrote:
> >> *************** typedef void (lto_free_section_data_f) (
> >> *** 346,355 ****
> >> Â struct lto_streamer_cache_d
> >> Â {
> >> Â Â /* The mapping between tree nodes and slots into the nodes array. Â */
> >> ! Â htab_t node_map;
> >> !
> >> ! Â /* Node map to store entries into. Â */
> >> ! Â alloc_pool node_map_entries;
> >>
> >> Â Â /* The nodes pickled so far. Â */
> >> Â Â VEC(tree,heap) *nodes;
> >> --- 346,352 ----
> >> Â struct lto_streamer_cache_d
> >> Â {
> >> Â Â /* The mapping between tree nodes and slots into the nodes array. Â */
> >> ! Â struct pointer_map_t GTY((skip)) *node_map;
> >
> > If you skip node_map you can end up with false entries for re-used
> > trees. Â So I don't think that's a good idea.
>
> Or we can safely mark the whole struct as non-GC (and also allocate
> it that way).
We already do (see lto-streamer.c:lto_streamer_cache_create; there's no
GTY marker on lto_streamer_cache_d. So Honza's original GTY annotation
there is superfluous.
-Nathan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 14:46 ` Richard Guenther
2011-05-02 14:49 ` Nathan Froyd
@ 2011-05-02 14:52 ` Michael Matz
1 sibling, 0 replies; 12+ messages in thread
From: Michael Matz @ 2011-05-02 14:52 UTC (permalink / raw)
To: Richard Guenther; +Cc: Jan Hubicka, gcc-patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 553 bytes --]
Hi,
On Mon, 2 May 2011, Richard Guenther wrote:
> >> Â Â /* The mapping between tree nodes and slots into the nodes array. Â */
> >> ! Â struct pointer_map_t GTY((skip)) *node_map;
> >
> > If you skip node_map you can end up with false entries for re-used
> > trees. Â So I don't think that's a good idea.
>
> Or we can safely mark the whole struct as non-GC (and also allocate
> it that way).
The whole struct (lto_streamer_cache_d) isn't marked for GC, and isn't
alloced in GC. The GTY((skip)) marker is superfluous.
Ciao,
Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 14:39 ` Richard Guenther
2011-05-02 14:46 ` Richard Guenther
@ 2011-05-02 14:57 ` Jan Hubicka
1 sibling, 0 replies; 12+ messages in thread
From: Jan Hubicka @ 2011-05-02 14:57 UTC (permalink / raw)
To: Richard Guenther; +Cc: Jan Hubicka, gcc-patches
>
> If you skip node_map you can end up with false entries for re-used
> trees. So I don't think that's a good idea.
The GTY marker is bogus there. I believed that the cache is GTY annotated
and then the skip would be safe, since whether is in the map is also in the
following array.
But it is not GTY marked, so OK with that marker removed?
Honza
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 14:35 ` Michael Matz
@ 2011-05-02 15:14 ` Richard Guenther
2011-05-02 15:16 ` Michael Matz
0 siblings, 1 reply; 12+ messages in thread
From: Richard Guenther @ 2011-05-02 15:14 UTC (permalink / raw)
To: Michael Matz; +Cc: Jan Hubicka, gcc-patches
On Mon, May 2, 2011 at 4:35 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Mon, 2 May 2011, Jan Hubicka wrote:
>
>> ! d_entry.base.from = t;
>> ! slot = htab_find_slot (cache->node_map, &d_entry, INSERT);
>> ! if (*slot == NULL)
>> {
>> /* Determine the next slot to use in the cache. */
>> if (insert_at_next_slot_p)
>> ix = VEC_length (tree, cache->nodes);
>> else
>> ix = *ix_p;
>> !
>> ! entry = (struct tree_int_map *)pool_alloc (cache->node_map_entries);
>> ! entry->base.from = t;
>> ! entry->to = ix;
>> ! *slot = entry;
>>
>> lto_streamer_cache_add_to_node_array (cache, ix, t);
>>
>> --- 348,367 ----
>> bool insert_at_next_slot_p)
>> {
>> void **slot;
>> unsigned ix;
>> bool existed_p;
>>
>> gcc_assert (t);
>>
>> ! slot = pointer_map_insert (cache->node_map, t);
>> ! if (!*slot)
>
> ix might legitimately be zero. Hence this transformation is not
> equivalent. You might want to enter ix+1 into the cache with the
> appropriate adjustment at read-out. Same for the other places.
Or not use index zero. Maybe better than also have to deal
with ix + 1 wrapping ...
Richard.
>
> Ciao,
> Michael.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 15:14 ` Richard Guenther
@ 2011-05-02 15:16 ` Michael Matz
2011-05-02 15:25 ` Jan Hubicka
2011-05-02 15:25 ` Richard Guenther
0 siblings, 2 replies; 12+ messages in thread
From: Michael Matz @ 2011-05-02 15:16 UTC (permalink / raw)
To: Richard Guenther; +Cc: Jan Hubicka, gcc-patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 841 bytes --]
Hi,
On Mon, 2 May 2011, Richard Guenther wrote:
> >> --- 348,367 ----
> >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â bool insert_at_next_slot_p)
> >> Â {
> >> Â Â void **slot;
> >> Â Â unsigned ix;
> >> Â Â bool existed_p;
> >>
> >> Â Â gcc_assert (t);
> >>
> >> ! Â slot = pointer_map_insert (cache->node_map, t);
> >> ! Â if (!*slot)
> >
> > ix might legitimately be zero. Â Hence this transformation is not
> > equivalent. Â You might want to enter ix+1 into the cache with the
> > appropriate adjustment at read-out. Â Same for the other places.
>
> Or not use index zero.
I never like these sentinals.
> Maybe better than also have to deal with ix + 1 wrapping ...
We don't handle ix wrapping, why should we now suddenly care about ix+1
wrapping?
Ciao,
Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 15:16 ` Michael Matz
2011-05-02 15:25 ` Jan Hubicka
@ 2011-05-02 15:25 ` Richard Guenther
1 sibling, 0 replies; 12+ messages in thread
From: Richard Guenther @ 2011-05-02 15:25 UTC (permalink / raw)
To: Michael Matz; +Cc: Jan Hubicka, gcc-patches
On Mon, May 2, 2011 at 5:15 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Mon, 2 May 2011, Richard Guenther wrote:
>
>> >> --- 348,367 ----
>> >> bool insert_at_next_slot_p)
>> >> {
>> >> void **slot;
>> >> unsigned ix;
>> >> bool existed_p;
>> >>
>> >> gcc_assert (t);
>> >>
>> >> ! slot = pointer_map_insert (cache->node_map, t);
>> >> ! if (!*slot)
>> >
>> > ix might legitimately be zero. Hence this transformation is not
>> > equivalent. You might want to enter ix+1 into the cache with the
>> > appropriate adjustment at read-out. Same for the other places.
>>
>> Or not use index zero.
>
> I never like these sentinals.
Well. Or use pointer_map_contains () here.
Richard.
>> Maybe better than also have to deal with ix + 1 wrapping ...
>
> We don't handle ix wrapping, why should we now suddenly care about ix+1
> wrapping?
>
> Ciao,
> Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 15:16 ` Michael Matz
@ 2011-05-02 15:25 ` Jan Hubicka
2011-05-02 15:28 ` Richard Guenther
2011-05-02 15:25 ` Richard Guenther
1 sibling, 1 reply; 12+ messages in thread
From: Jan Hubicka @ 2011-05-02 15:25 UTC (permalink / raw)
To: Michael Matz; +Cc: Richard Guenther, Jan Hubicka, gcc-patches
> Hi,
>
> On Mon, 2 May 2011, Richard Guenther wrote:
>
> > >> --- 348,367 ----
> > >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â bool insert_at_next_slot_p)
> > >> Â {
> > >> Â Â void **slot;
> > >> Â Â unsigned ix;
> > >> Â Â bool existed_p;
> > >>
> > >> Â Â gcc_assert (t);
> > >>
> > >> ! Â slot = pointer_map_insert (cache->node_map, t);
> > >> ! Â if (!*slot)
> > >
> > > ix might legitimately be zero. Â Hence this transformation is not
> > > equivalent. Â You might want to enter ix+1 into the cache with the
> > > appropriate adjustment at read-out. Â Same for the other places.
> >
> > Or not use index zero.
>
> I never like these sentinals.
>
> > Maybe better than also have to deal with ix + 1 wrapping ...
>
> We don't handle ix wrapping, why should we now suddenly care about ix+1
> wrapping?
Duh, I actually intended to implement the ix+1 wrapping, but got dragged into discussions
about thunks ;)
I am testing the following patch. Seems sane?
Honza
Index: lto-streamer.c
===================================================================
*** lto-streamer.c (revision 173251)
--- lto-streamer.c (working copy)
*************** lto_streamer_cache_insert_1 (struct lto_
*** 348,373 ****
bool insert_at_next_slot_p)
{
void **slot;
- struct tree_int_map d_entry, *entry;
unsigned ix;
bool existed_p;
gcc_assert (t);
! d_entry.base.from = t;
! slot = htab_find_slot (cache->node_map, &d_entry, INSERT);
! if (*slot == NULL)
{
/* Determine the next slot to use in the cache. */
if (insert_at_next_slot_p)
ix = VEC_length (tree, cache->nodes);
else
ix = *ix_p;
!
! entry = (struct tree_int_map *)pool_alloc (cache->node_map_entries);
! entry->base.from = t;
! entry->to = ix;
! *slot = entry;
lto_streamer_cache_add_to_node_array (cache, ix, t);
--- 348,367 ----
bool insert_at_next_slot_p)
{
void **slot;
unsigned ix;
bool existed_p;
gcc_assert (t);
! slot = pointer_map_insert (cache->node_map, t);
! if (!*slot)
{
/* Determine the next slot to use in the cache. */
if (insert_at_next_slot_p)
ix = VEC_length (tree, cache->nodes);
else
ix = *ix_p;
! *slot = (void *)(size_t) (ix + 1);
lto_streamer_cache_add_to_node_array (cache, ix, t);
*************** lto_streamer_cache_insert_1 (struct lto_
*** 376,383 ****
}
else
{
! entry = (struct tree_int_map *) *slot;
! ix = entry->to;
if (!insert_at_next_slot_p && ix != *ix_p)
{
--- 370,376 ----
}
else
{
! ix = (size_t) *slot - 1;
if (!insert_at_next_slot_p && ix != *ix_p)
{
*************** lto_streamer_cache_lookup (struct lto_st
*** 442,455 ****
unsigned *ix_p)
{
void **slot;
- struct tree_int_map d_slot;
bool retval;
unsigned ix;
gcc_assert (t);
! d_slot.base.from = t;
! slot = htab_find_slot (cache->node_map, &d_slot, NO_INSERT);
if (slot == NULL)
{
retval = false;
--- 435,446 ----
unsigned *ix_p)
{
void **slot;
bool retval;
unsigned ix;
gcc_assert (t);
! slot = pointer_map_contains (cache->node_map, t);
if (slot == NULL)
{
retval = false;
*************** lto_streamer_cache_lookup (struct lto_st
*** 458,464 ****
else
{
retval = true;
! ix = ((struct tree_int_map *) *slot)->to;
}
if (ix_p)
--- 449,455 ----
else
{
retval = true;
! ix = (size_t) *slot - 1;
}
if (ix_p)
*************** lto_streamer_cache_create (void)
*** 608,618 ****
cache = XCNEW (struct lto_streamer_cache_d);
! cache->node_map = htab_create (101, tree_int_map_hash, tree_int_map_eq, NULL);
!
! cache->node_map_entries = create_alloc_pool ("node map",
! sizeof (struct tree_int_map),
! 100);
/* Load all the well-known tree nodes that are always created by
the compiler on startup. This prevents writing them out
--- 599,605 ----
cache = XCNEW (struct lto_streamer_cache_d);
! cache->node_map = pointer_map_create ();
/* Load all the well-known tree nodes that are always created by
the compiler on startup. This prevents writing them out
*************** lto_streamer_cache_delete (struct lto_st
*** 636,643 ****
if (c == NULL)
return;
! htab_delete (c->node_map);
! free_alloc_pool (c->node_map_entries);
VEC_free (tree, heap, c->nodes);
free (c);
}
--- 623,629 ----
if (c == NULL)
return;
! pointer_map_destroy (c->node_map);
VEC_free (tree, heap, c->nodes);
free (c);
}
Index: lto-streamer.h
===================================================================
*** lto-streamer.h (revision 173251)
--- lto-streamer.h (working copy)
*************** typedef void (lto_free_section_data_f) (
*** 346,355 ****
struct lto_streamer_cache_d
{
/* The mapping between tree nodes and slots into the nodes array. */
! htab_t node_map;
!
! /* Node map to store entries into. */
! alloc_pool node_map_entries;
/* The nodes pickled so far. */
VEC(tree,heap) *nodes;
--- 346,352 ----
struct lto_streamer_cache_d
{
/* The mapping between tree nodes and slots into the nodes array. */
! struct pointer_map_t *node_map;
/* The nodes pickled so far. */
VEC(tree,heap) *nodes;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Turn streamer cache to pointer_map
2011-05-02 15:25 ` Jan Hubicka
@ 2011-05-02 15:28 ` Richard Guenther
0 siblings, 0 replies; 12+ messages in thread
From: Richard Guenther @ 2011-05-02 15:28 UTC (permalink / raw)
To: Jan Hubicka; +Cc: Michael Matz, gcc-patches
2011/5/2 Jan Hubicka <hubicka@ucw.cz>:
>> Hi,
>>
>> On Mon, 2 May 2011, Richard Guenther wrote:
>>
>> > >> --- 348,367 ----
>> > >> bool insert_at_next_slot_p)
>> > >> {
>> > >> void **slot;
>> > >> unsigned ix;
>> > >> bool existed_p;
>> > >>
>> > >> gcc_assert (t);
>> > >>
>> > >> ! slot = pointer_map_insert (cache->node_map, t);
>> > >> ! if (!*slot)
>> > >
>> > > ix might legitimately be zero. Hence this transformation is not
>> > > equivalent. You might want to enter ix+1 into the cache with the
>> > > appropriate adjustment at read-out. Same for the other places.
>> >
>> > Or not use index zero.
>>
>> I never like these sentinals.
>>
>> > Maybe better than also have to deal with ix + 1 wrapping ...
>>
>> We don't handle ix wrapping, why should we now suddenly care about ix+1
>> wrapping?
>
>
> Duh, I actually intended to implement the ix+1 wrapping, but got dragged into discussions
> about thunks ;)
>
> I am testing the following patch. Seems sane?
Ok if it works.
Richard.
> Honza
>
> Index: lto-streamer.c
> ===================================================================
> *** lto-streamer.c (revision 173251)
> --- lto-streamer.c (working copy)
> *************** lto_streamer_cache_insert_1 (struct lto_
> *** 348,373 ****
> bool insert_at_next_slot_p)
> {
> void **slot;
> - struct tree_int_map d_entry, *entry;
> unsigned ix;
> bool existed_p;
>
> gcc_assert (t);
>
> ! d_entry.base.from = t;
> ! slot = htab_find_slot (cache->node_map, &d_entry, INSERT);
> ! if (*slot == NULL)
> {
> /* Determine the next slot to use in the cache. */
> if (insert_at_next_slot_p)
> ix = VEC_length (tree, cache->nodes);
> else
> ix = *ix_p;
> !
> ! entry = (struct tree_int_map *)pool_alloc (cache->node_map_entries);
> ! entry->base.from = t;
> ! entry->to = ix;
> ! *slot = entry;
>
> lto_streamer_cache_add_to_node_array (cache, ix, t);
>
> --- 348,367 ----
> bool insert_at_next_slot_p)
> {
> void **slot;
> unsigned ix;
> bool existed_p;
>
> gcc_assert (t);
>
> ! slot = pointer_map_insert (cache->node_map, t);
> ! if (!*slot)
> {
> /* Determine the next slot to use in the cache. */
> if (insert_at_next_slot_p)
> ix = VEC_length (tree, cache->nodes);
> else
> ix = *ix_p;
> ! *slot = (void *)(size_t) (ix + 1);
>
> lto_streamer_cache_add_to_node_array (cache, ix, t);
>
> *************** lto_streamer_cache_insert_1 (struct lto_
> *** 376,383 ****
> }
> else
> {
> ! entry = (struct tree_int_map *) *slot;
> ! ix = entry->to;
>
> if (!insert_at_next_slot_p && ix != *ix_p)
> {
> --- 370,376 ----
> }
> else
> {
> ! ix = (size_t) *slot - 1;
>
> if (!insert_at_next_slot_p && ix != *ix_p)
> {
> *************** lto_streamer_cache_lookup (struct lto_st
> *** 442,455 ****
> unsigned *ix_p)
> {
> void **slot;
> - struct tree_int_map d_slot;
> bool retval;
> unsigned ix;
>
> gcc_assert (t);
>
> ! d_slot.base.from = t;
> ! slot = htab_find_slot (cache->node_map, &d_slot, NO_INSERT);
> if (slot == NULL)
> {
> retval = false;
> --- 435,446 ----
> unsigned *ix_p)
> {
> void **slot;
> bool retval;
> unsigned ix;
>
> gcc_assert (t);
>
> ! slot = pointer_map_contains (cache->node_map, t);
> if (slot == NULL)
> {
> retval = false;
> *************** lto_streamer_cache_lookup (struct lto_st
> *** 458,464 ****
> else
> {
> retval = true;
> ! ix = ((struct tree_int_map *) *slot)->to;
> }
>
> if (ix_p)
> --- 449,455 ----
> else
> {
> retval = true;
> ! ix = (size_t) *slot - 1;
> }
>
> if (ix_p)
> *************** lto_streamer_cache_create (void)
> *** 608,618 ****
>
> cache = XCNEW (struct lto_streamer_cache_d);
>
> ! cache->node_map = htab_create (101, tree_int_map_hash, tree_int_map_eq, NULL);
> !
> ! cache->node_map_entries = create_alloc_pool ("node map",
> ! sizeof (struct tree_int_map),
> ! 100);
>
> /* Load all the well-known tree nodes that are always created by
> the compiler on startup. This prevents writing them out
> --- 599,605 ----
>
> cache = XCNEW (struct lto_streamer_cache_d);
>
> ! cache->node_map = pointer_map_create ();
>
> /* Load all the well-known tree nodes that are always created by
> the compiler on startup. This prevents writing them out
> *************** lto_streamer_cache_delete (struct lto_st
> *** 636,643 ****
> if (c == NULL)
> return;
>
> ! htab_delete (c->node_map);
> ! free_alloc_pool (c->node_map_entries);
> VEC_free (tree, heap, c->nodes);
> free (c);
> }
> --- 623,629 ----
> if (c == NULL)
> return;
>
> ! pointer_map_destroy (c->node_map);
> VEC_free (tree, heap, c->nodes);
> free (c);
> }
> Index: lto-streamer.h
> ===================================================================
> *** lto-streamer.h (revision 173251)
> --- lto-streamer.h (working copy)
> *************** typedef void (lto_free_section_data_f) (
> *** 346,355 ****
> struct lto_streamer_cache_d
> {
> /* The mapping between tree nodes and slots into the nodes array. */
> ! htab_t node_map;
> !
> ! /* Node map to store entries into. */
> ! alloc_pool node_map_entries;
>
> /* The nodes pickled so far. */
> VEC(tree,heap) *nodes;
> --- 346,352 ----
> struct lto_streamer_cache_d
> {
> /* The mapping between tree nodes and slots into the nodes array. */
> ! struct pointer_map_t *node_map;
>
> /* The nodes pickled so far. */
> VEC(tree,heap) *nodes;
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-05-02 15:28 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-02 14:16 Turn streamer cache to pointer_map Jan Hubicka
2011-05-02 14:35 ` Michael Matz
2011-05-02 15:14 ` Richard Guenther
2011-05-02 15:16 ` Michael Matz
2011-05-02 15:25 ` Jan Hubicka
2011-05-02 15:28 ` Richard Guenther
2011-05-02 15:25 ` Richard Guenther
2011-05-02 14:39 ` Richard Guenther
2011-05-02 14:46 ` Richard Guenther
2011-05-02 14:49 ` Nathan Froyd
2011-05-02 14:52 ` Michael Matz
2011-05-02 14:57 ` Jan Hubicka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).