On Tue, 3 May 2011, Jan Hubicka wrote:
> Hi,
> I always considered the cgrpah_node_set/varpool_node_set to be overengineered
> but they also turned out to be quite ineffective since we do quite a lot of
> queries into them during stremaing out.
>
> This patch moves them to pointer_map, like I did for streamer cache. While
> doing so I needed to get the structure out of GGC memory since pointer_map is
> not ggc firendly. This is not a deal at all, because the sets can only point
> to
> live cgraph/varpool entries anyway. Pointing to removed ones would lead to
> spectacular failures in any case.
>
> Bootstrapped/regtested x86_64-linux, OK?
Umm, I wonder why ...
> + cgraph_node_set_add (cgraph_node_set set, struct cgraph_node *node)
> + {
> + void **slot;
> +
> + slot = pointer_map_insert (set->map, node);
> +
> + if (*slot)
> + {
> + int index = (size_t) *slot - 1;
> + gcc_checking_assert ((VEC_index (cgraph_node_ptr, set->nodes, index)
> + == node));
> + return;
> + }
> +
> + *slot = (void *)(size_t) (VEC_length (cgraph_node_ptr, set->nodes) + 1);
> +
> + /* Insert into node vector. */
> + VEC_safe_push (cgraph_node_ptr, heap, set->nodes, node);
We have both a vector and a pointer-map. Why not simply use a
pointer-map only?! I see this may need more re-structuring, eventually
adding an iterator interface to pointer-sets/maps (which would be
nice anyway).
It also makes me think again that why do we have both a cgraph
and a varpool set ... at least when you now deal with a non-GC
data structure you could as well use a vector of void * and
provide macros doing the casting for you, unifying the implementation
itself.
Well, I suppose most of that can be done as a followup (when
eventually the symtab arrives and varpool and cgraph nodes merge anyway).
Thus, ok for now.
Thanks,
Richard.
