[Bug tree-optimization/58028] New: [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[Bug tree-optimization/58028] New: [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[dominiq at lps dot ens.fr]

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

            Bug ID: 58028
           Summary: [4.9 Regression] Several failures in libgomp.graphite
                    after revision 200946
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dominiq at lps dot ens.fr
                CC: grosser at gcc dot gnu.org

At revision 200946 the following failures (not seen in r200874) appeared (see
also http://gcc.gnu.org/ml/gcc-testresults/2013-07/msg02544.html ):

FAIL: libgomp.graphite/bounds.c scan-tree-dump-times graphite "0 loops carried
no dependency" 1
FAIL: libgomp.graphite/force-parallel-4.c execution test
FAIL: libgomp.graphite/force-parallel-4.c scan-tree-dump-times graphite "2
loops carried no dependency" 1
FAIL: libgomp.graphite/force-parallel-6.c scan-tree-dump-times graphite "1
loops carried no dependency" 1
FAIL: libgomp.graphite/pr41118.c scan-tree-dump-times optimized "loopfn" 0

with -m32 and -m64 (for r201008, the libgomp.graphite/force-parallel-4.c
execution test failed only with -m32, for r201274 and newer it fails with both
-m32/-m64).

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[ro at gcc dot gnu.org]

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

Rainer Orth <ro at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-08-09
                 CC|                            |ro at gcc dot gnu.org
   Target Milestone|---                         |4.9.0
     Ever confirmed|0                           |1

--- Comment #1 from Rainer Orth <ro at gcc dot gnu.org> ---
Confirmed: also seen on Solaris/SPARC and x86.

  Rainer

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[dominiq at lps dot ens.fr]

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

Dominique d'Humieres <dominiq at lps dot ens.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenther at suse dot de

--- Comment #2 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
What is the "fix or revert" policy?

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|rguenther at suse dot de           |rguenth at gcc dot gnu.org
             Blocks|                            |54094

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
First identify the revision that caused this.  I would guess it's

2013-07-13  Tobias Grosser  <tobias@grosser.es>

       PR tree-optimization/54094
       * graphite-clast-to-gimple.c (translate_clast_for_loop): Derive the
         scheduling dimension for the parallelism check from the polyhedral
         information in the AST.
       * graphite-dependences.c (carries_deps): Do not assume the schedule is
         in 2D + 1 form.

?

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[graham at opengamma dot com]

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

Graham Markall <graham at opengamma dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |graham at opengamma dot com

--- Comment #4 from Graham Markall <graham at opengamma dot com> ---
I also came across this on x86_64. Reverting the commit suggested by Richard
Biener seems to fix these failures on my system (however, the minimal testcase
from PR 54094 causes an ICE again, as may be expected). This was with version:

$ gcc --version
gcc (GCC) 4.9.0 20131119 (experimental)

That I built from:

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@205019
138bc75d-0d04-0410-961f-82ee72b054a4

Unfortunately I am not knowledgeable enough to provide a proper fix - however,
I would be happy to assist in testing such a fix.

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Is graphite, given its lack of active maintainance, worth the P1 importance?

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
The testsuite failures are.  We have to do sth about them.

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[dominiq at lps dot ens.fr]

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

--- Comment #7 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> The testsuite failures are.  We have to do sth about them.

Revert r200946?

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[rguenther at suse dot de]

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

--- Comment #8 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 29 Jan 2014, dominiq at lps dot ens.fr wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58028
> 
> --- Comment #7 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> > The testsuite failures are.  We have to do sth about them.
> 
> Revert r200946?

Probably - the

+       * graphite-dependences.c (carries_deps): Do not assume the 
schedule is
+         in 2D + 1 form.

part looks wrong.

-  idx = 2 * depth + 1;
-  for (i = 0; i < idx; i++)
+  for (i = 0; i < depth - 1; i++)

we now iterate over less dimensions than before.

I'd say we should simply check whether the loop _is_ in 2D + 1 form
at

+  isl_set *domain = isl_set_from_cloog_domain (stmt->domain);
+  int scheduling_dim = isl_set_n_dim (domain);
+
   if (flag_loop_parallelize_all
-      && loop_is_parallel_p (loop, bb_pbb_mapping, level))
+      && loop_is_parallel_p (loop, bb_pbb_mapping, scheduling_dim))
     loop->can_be_parallel = true;

thus

   if (flag_loop_parallelize_all
       && scheduling_dim == 2 * level + 1
       && loop_is_parallel_p (loop, bb_pbb_mapping, level)
     loop->can_be_parallel = true;

no time to check whether reverting the other hunk plus this will
resolve the bug the revision fixed and restores the testcases.

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[grosser at gcc dot gnu.org]

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

--- Comment #9 from Tobias Grosser <grosser at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #8)
> On Wed, 29 Jan 2014, dominiq at lps dot ens.fr wrote:
> 
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58028
> > 
> > --- Comment #7 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> > > The testsuite failures are.  We have to do sth about them.
> > 
> > Revert r200946?
> 
> Probably - the
> 
> +       * graphite-dependences.c (carries_deps): Do not assume the 
> schedule is
> +         in 2D + 1 form.
> 
> part looks wrong.
> 
> -  idx = 2 * depth + 1;
> -  for (i = 0; i < idx; i++)
> +  for (i = 0; i < depth - 1; i++)
> 
> we now iterate over less dimensions than before.

Only in cases where the schedule is not in 2D + 1 form. In case the schedule
is in 2D + 1 form 

int scheduling_dim = isl_set_n_dim (domain);

will be equal to 2 * depth + 1.

> I'd say we should simply check whether the loop _is_ in 2D + 1 form
> at
> 
> +  isl_set *domain = isl_set_from_cloog_domain (stmt->domain);
> +  int scheduling_dim = isl_set_n_dim (domain);
> +
>    if (flag_loop_parallelize_all
> -      && loop_is_parallel_p (loop, bb_pbb_mapping, level))
> +      && loop_is_parallel_p (loop, bb_pbb_mapping, scheduling_dim))
>      loop->can_be_parallel = true;
>  
> thus
> 
>    if (flag_loop_parallelize_all
>        && scheduling_dim == 2 * level + 1
>        && loop_is_parallel_p (loop, bb_pbb_mapping, level)
>      loop->can_be_parallel = true;

The change you propose seems conservatively correct, as in that loops that
are not in 2D + 1 form are not detected to be parallel. This change may hide
the bug, but I don't see any bug it solves.

> no time to check whether reverting the other hunk plus this will
> resolve the bug the revision fixed and restores the testcases.

I assume it will fix the crash, but it will not detect parallellism in none
2D+1 loops, something the isl scheduler happily creates.

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[rguenther at suse dot de]

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

--- Comment #10 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 29 Jan 2014, grosser at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58028
> 
> --- Comment #9 from Tobias Grosser <grosser at gcc dot gnu.org> ---
> (In reply to rguenther@suse.de from comment #8)
> > On Wed, 29 Jan 2014, dominiq at lps dot ens.fr wrote:
> > 
> > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58028
> > > 
> > > --- Comment #7 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> > > > The testsuite failures are.  We have to do sth about them.
> > > 
> > > Revert r200946?
> > 
> > Probably - the
> > 
> > +       * graphite-dependences.c (carries_deps): Do not assume the 
> > schedule is
> > +         in 2D + 1 form.
> > 
> > part looks wrong.
> > 
> > -  idx = 2 * depth + 1;
> > -  for (i = 0; i < idx; i++)
> > +  for (i = 0; i < depth - 1; i++)
> > 
> > we now iterate over less dimensions than before.
> 
> Only in cases where the schedule is not in 2D + 1 form. In case the schedule
> is in 2D + 1 form 
> 
> int scheduling_dim = isl_set_n_dim (domain);
> 
> will be equal to 2 * depth + 1.
> 
> > I'd say we should simply check whether the loop _is_ in 2D + 1 form
> > at
> > 
> > +  isl_set *domain = isl_set_from_cloog_domain (stmt->domain);
> > +  int scheduling_dim = isl_set_n_dim (domain);
> > +
> >    if (flag_loop_parallelize_all
> > -      && loop_is_parallel_p (loop, bb_pbb_mapping, level))
> > +      && loop_is_parallel_p (loop, bb_pbb_mapping, scheduling_dim))
> >      loop->can_be_parallel = true;
> >  
> > thus
> > 
> >    if (flag_loop_parallelize_all
> >        && scheduling_dim == 2 * level + 1
> >        && loop_is_parallel_p (loop, bb_pbb_mapping, level)
> >      loop->can_be_parallel = true;
> 
> The change you propose seems conservatively correct, as in that loops that
> are not in 2D + 1 form are not detected to be parallel. This change may hide
> the bug, but I don't see any bug it solves.

It may solve the fallout from your change - I don't have enough
graphite knowledge to fix a real bug there ;)

> > no time to check whether reverting the other hunk plus this will
> > resolve the bug the revision fixed and restores the testcases.
> 
> I assume it will fix the crash, but it will not detect parallellism in none
> 2D+1 loops, something the isl scheduler happily creates.

Sure.  We'd be much happier if the real bug was fixed, maybe you can
give it a try?  The

FAIL: libgomp.graphite/force-parallel-4.c execution test
FAIL: libgomp.graphite/force-parallel-4.c scan-tree-dump-times graphite "2 
loops
 carried no dependency" 1

FAIL suggests that we miss some dependency now.

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[grosser at gcc dot gnu.org]

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

Tobias Grosser <grosser at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #11 from Tobias Grosser <grosser at gcc dot gnu.org> ---
Thanks Richi for starting to look into this. As posted in the other bug report
Mircea, will be taking over the maintenance work with graphite, looking into
this bug report with priority. I will be helping him to come to speed with
graphite.

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[rguenth at gcc dot gnu.org]

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

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
That's good news!

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

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

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.

[Bug tree-optimization/58028] [4.9 Regression] Several failures in libgomp.graphite after revision 200946

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

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

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
Author: rguenth
Date: Mon Mar  3 11:39:22 2014
New Revision: 208273

URL: http://gcc.gnu.org/viewcvs?rev=208273&root=gcc&view=rev
Log:
2014-03-03  Tobias Grosser  <tobias@grosser.es>
    Mircea Namolaru  <mircea.namolaru@inria.fr>

    PR tree-optimization/58028
    * graphite-clast-to-gimple.c (set_cloog_options): Don't remove
    scalar dimensions.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/graphite-clast-to-gimple.c