Patchwork 19/n: trans-mem: middle end/misc patches (LAST PATCH)

login
register
mail settings
Submitter Aldy Hernandez
Date Nov. 7, 2011, 5:55 a.m.
Message ID <4EB772DD.7020904@redhat.com>
Download mbox | patch
Permalink /patch/124005/
State New
Headers show

Comments

Aldy Hernandez - Nov. 7, 2011, 5:55 a.m.
> False.  You get the equivalent of bootstrap comparison mismatches.
> If we actually used tm during the bootstrap.
>
> The simplest thing to do is to change the hash this table uses.
> E.g. use the DECL_UID right from the start, rather than the pointer.

Woah!  Can it be that easy?  That's as easy as changing the hash, no 
conversion necessary.

OK for branch?

	* varasm.c (record_tm_clone_pair): Use DECL_UID as hash.
	(get_tm_clone_pair): Same.
Richard Guenther - Nov. 7, 2011, 9:49 a.m.
On Mon, Nov 7, 2011 at 6:55 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
>
>> False.  You get the equivalent of bootstrap comparison mismatches.
>> If we actually used tm during the bootstrap.
>>
>> The simplest thing to do is to change the hash this table uses.
>> E.g. use the DECL_UID right from the start, rather than the pointer.
>
> Woah!  Can it be that easy?  That's as easy as changing the hash, no
> conversion necessary.
>
> OK for branch?

This won't work - DECL_UIDs are not stable -g vs. -g0 - only their
_order_ is stable - thus you won't get comparison fails with code generated
dependent on DECL_UID order, but you will if you depend on the DECL_UID
value (which you do by using it as a hash).

And we will still generate different object files based on garbage collector
settings this way - GC can shrink hashtables, causing re-hashing,
which changes the order of the elements.  It also causes re-ordering
with slight unrelated code changes (but if you say at runtime we always
sort the thing that might not be an issue).

Thus, the patch isn't a fix to get a stable order (you can't get
that for hashtable walks).  A quick fix is to collect the elements
into a VEC and qsort that after some stable key (like the DECL_UID).

Thanks,
Richard.

>        * varasm.c (record_tm_clone_pair): Use DECL_UID as hash.
>        (get_tm_clone_pair): Same.
>
> Index: varasm.c
> ===================================================================
> --- varasm.c    (revision 181067)
> +++ varasm.c    (working copy)
> @@ -5875,7 +5875,7 @@ record_tm_clone_pair (tree o, tree n)
>     tm_clone_pairs = htab_create_ggc (32, tree_map_hash, tree_map_eq, 0);
>
>   h = ggc_alloc_tree_map ();
> -  h->hash = htab_hash_pointer (o);
> +  h->hash = DECL_UID (o);
>   h->base.from = o;
>   h->to = n;
>
> @@ -5892,7 +5892,7 @@ get_tm_clone_pair (tree o)
>       struct tree_map *h, in;
>
>       in.base.from = o;
> -      in.hash = htab_hash_pointer (o);
> +      in.hash = DECL_UID (o);
>       h = (struct tree_map *) htab_find_with_hash (tm_clone_pairs,
>                                                   &in, in.hash);
>       if (h)
>

Patch

Index: varasm.c
===================================================================
--- varasm.c	(revision 181067)
+++ varasm.c	(working copy)
@@ -5875,7 +5875,7 @@  record_tm_clone_pair (tree o, tree n)
      tm_clone_pairs = htab_create_ggc (32, tree_map_hash, tree_map_eq, 0);

    h = ggc_alloc_tree_map ();
-  h->hash = htab_hash_pointer (o);
+  h->hash = DECL_UID (o);
    h->base.from = o;
    h->to = n;

@@ -5892,7 +5892,7 @@  get_tm_clone_pair (tree o)
        struct tree_map *h, in;

        in.base.from = o;
-      in.hash = htab_hash_pointer (o);
+      in.hash = DECL_UID (o);
        h = (struct tree_map *) htab_find_with_hash (tm_clone_pairs,
  						   &in, in.hash);
        if (h)