[Bug tree-optimization/42641] New: Random code-generation differences with GRAPHITE

[Bug tree-optimization/42641] New: Random code-generation differences with GRAPHITE

[rguenth at gcc dot gnu dot org]

Graphites code generation depends on virtual addresses because we traverse
hashtables (at least) when inserting guard phis from sese.c:insert_guard_phis
which elements are hashed by pointer, sese.c:rename_map_elt_info

A fix is to hash the SSA name instead:

Index: sese.c
===================================================================
--- sese.c      (revision 155678)
+++ sese.c      (working copy)
@@ -78,7 +78,7 @@ debug_rename_map (htab_t map)
 hashval_t
 rename_map_elt_info (const void *elt)
 {
-  return htab_hash_pointer (((const struct rename_map_elt_s *)
elt)->old_name);
+  return SSA_NAME_VERSION ((const struct rename_map_elt_s *) elt)->old_name;
 }

 /* Compares database elements E1 and E2.  */


there is a lot more pointer-hashing done and a lot of hashtab traversals.
They all need to be audited.


-- 
           Summary: Random code-generation differences with GRAPHITE
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: critical
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: rguenth at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[zsojka at seznam dot cz]

------- Comment #1 from zsojka at seznam dot cz  2010-01-07 12:43 -------
Created an attachment (id=19496)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=19496&action=view)
testcase (not reduced)

While reducing this testcase with delta, different -fcompare-debug problem
appeared (with non-random behaviour, failing with just -O1), so I won't upload
the reduced version here.

Fails randomly with "-O2 -fgraphite-identity -fcompare-debug", x86_64, r156680:
# echo 1 > /proc/sys/kernel/randomize_va_space
$ /mnt/svn/gcc-trunk/binary-155680-lto/bin/g++ -O2 -fgraphite-identity
-fcompare-debug -c dbg_helpers-noempty.ii
$ /mnt/svn/gcc-trunk/binary-155680-lto/bin/g++ -O2 -fgraphite-identity
-fcompare-debug -c dbg_helpers-noempty.ii
$ /mnt/svn/gcc-trunk/binary-155680-lto/bin/g++ -O2 -fgraphite-identity
-fcompare-debug -c dbg_helpers-noempty.ii
g++: dbg_helpers-noempty.ii: -fcompare-debug failure

Also fails randomly with:
-O1 -fcompare-debug -ftree-pre -ftree-vectorize -fgraphite-identity

I tried to compile gcc with patch from comment #0, but it fails with:
../../gcc/sese.c: In function ‘rename_map_elt_info’:
../../gcc/sese.c:81: error: ‘const struct rename_map_elt_s’ has no
member named ‘base’
../../gcc/sese.c:81: warning: passing argument 1 of
‘tree_check_failed’ from incompatible pointer type
../../gcc/sese.c:81: error: ‘const struct rename_map_elt_s’ has no
member named ‘ssa_name’
../../gcc/sese.c:82: warning: control reaches end of non-void function


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[rguenth at gcc dot gnu dot org]

------- Comment #2 from rguenth at gcc dot gnu dot org  2010-01-07 14:13 -------
I can reproduce the problem with the testcase and the patch seems to fix it
(with the typo fixed and as far as you can test for fixed random behavior...).

I'm going to bootstrap, test and install it.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |rguenth at gcc dot gnu dot
                   |dot org                     |org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2010-01-07 14:13:52
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[rguenth at gcc dot gnu dot org]

------- Comment #3 from rguenth at gcc dot gnu dot org  2010-01-07 16:07 -------
Subject: Bug 42641

Author: rguenth
Date: Thu Jan  7 16:07:17 2010
New Revision: 155695

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=155695
Log:
2010-01-07  Richard Guenther  <rguenther@suse.de>

        PR tree-optimization/42641
        * sese.c (rename_map_elt_info): Use the SSA name version, do
        not hash pointers.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/sese.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[rguenth at gcc dot gnu dot org]

------- Comment #4 from rguenth at gcc dot gnu dot org  2010-01-07 16:09 -------
Fixed.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[zsojka at seznam dot cz]

------- Comment #5 from zsojka at seznam dot cz  2010-01-07 17:53 -------
Patch from comment #0 works for me too (sorry for the delay, it took some time
to recompile after realising where is the typo in that patch).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[sebpop at gmail dot com]

------- Comment #6 from sebpop at gmail dot com  2010-01-07 17:58 -------
Subject: Re:  Random code-generation differences 
        with GRAPHITE

After your change, there remains three users of htab_hash_pointer in graphite:

In if_region_set_false_region, there is a use of htab_hash_pointer,
but that matches the use of the loops->exits htab as also used in
get_exit_descriptions.

The next two, are:

hashval_t
ivtype_map_elt_info (const void *elt)
{
  return htab_hash_pointer (((const struct ivtype_map_elt_s *) elt)->cloog_iv);
}

static inline hashval_t
clast_name_index_elt_info (const void *elt)
{
  return htab_hash_pointer (((const struct clast_name_index *) elt)->name);
}

and they are a bit more difficult to change, as it is the interface
with CLooG that uses a "char *" to identify loop induction variables.
In both cases, we're hashing on that string identifier.

Should these two functions be changed as well?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[spop at gcc dot gnu dot org]

------- Comment #7 from spop at gcc dot gnu dot org  2010-01-07 18:01 -------
Subject: Bug 42641

Author: spop
Date: Thu Jan  7 18:01:28 2010
New Revision: 155700

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=155700
Log:
Do not hash pointers.

2010-01-07  Richard Guenther  <rguenther@suse.de>

        PR tree-optimization/42641
        * sese.c (rename_map_elt_info): Use the SSA name version, do
        not hash pointers.

Modified:
    branches/graphite/gcc/ChangeLog.graphite
    branches/graphite/gcc/sese.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[jakub at gcc dot gnu dot org]

------- Comment #8 from jakub at gcc dot gnu dot org  2010-01-07 21:10 -------
htab_hash_pointer is fine if a hash table is never traversed, or such traversal
can't affect code generation.  E.g. graphite has some debug_* routines that
traverse such hash tables, that's fine, they aren't called at all during
compilation except for debugging sessions.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[sebpop at gmail dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 638 bytes --]



------- Comment #9 from sebpop at gmail dot com  2010-01-07 21:30 -------
Subject: Re:  Random code-generation differences 
        with GRAPHITE

> htab_hash_pointer is fine if a hash table is never traversed, or such traversal
> can't affect code generation.  E.g. graphite has some debug_* routines that
> traverse such hash tables, that's fine, they aren't called at all during
> compilation except for debugging sessions.

Ok, thanks for the detailed explanation.

The two other htabs using htab_hash_pointer
ivtype_map_elt_info and clast_name_index_elt_info are safe.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641

Re: [Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[Sebastian Pop]

> htab_hash_pointer is fine if a hash table is never traversed, or such traversal
> can't affect code generation.  E.g. graphite has some debug_* routines that
> traverse such hash tables, that's fine, they aren't called at all during
> compilation except for debugging sessions.

Ok, thanks for the detailed explanation.

The two other htabs using htab_hash_pointer
ivtype_map_elt_info and clast_name_index_elt_info are safe.

[Bug tree-optimization/42641] Random code-generation differences with GRAPHITE

[rguenther at suse dot de]

------- Comment #10 from rguenther at suse dot de  2010-01-08 11:09 -------
Subject: Re:  Random code-generation differences
 with GRAPHITE

On Thu, 7 Jan 2010, sebpop at gmail dot com wrote:

> 
> 
> ------- Comment #6 from sebpop at gmail dot com  2010-01-07 17:58 -------
> Subject: Re:  Random code-generation differences 
>         with GRAPHITE
> 
> After your change, there remains three users of htab_hash_pointer in graphite:
> 
> In if_region_set_false_region, there is a use of htab_hash_pointer,
> but that matches the use of the loops->exits htab as also used in
> get_exit_descriptions.
> 
> The next two, are:
> 
> hashval_t
> ivtype_map_elt_info (const void *elt)
> {
>   return htab_hash_pointer (((const struct ivtype_map_elt_s *) elt)->cloog_iv);
> }
> 
> static inline hashval_t
> clast_name_index_elt_info (const void *elt)
> {
>   return htab_hash_pointer (((const struct clast_name_index *) elt)->name);
> }
> 
> and they are a bit more difficult to change, as it is the interface
> with CLooG that uses a "char *" to identify loop induction variables.
> In both cases, we're hashing on that string identifier.
> 
> Should these two functions be changed as well?

If you ever end up traversing those hash tables then yes, I suggest
to use htab_hash_string instead.  If you are not traversing them you
might want to consider using the somewhat more efficient pointer_map
structure (but that's only an optimization then).

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42641