[PATCH] Fix bug 59586

[Tobias Grosser]

I worked with Roman on this patch and believe it is technically correct.
Some tiny comments inline.

Tobias


On 03/08/2014 10:57 AM, Roman Gareev wrote:
> This patch fixes the following bug:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59586.

I would write PR59586. This should make this patch show up in the bug 
tracker.

> The segfault is caused by NULL arguments passed to compute_deps by
> loop_level_carries_dependences.
> This causes an assignment of NULL values to the no_source parameters of
> compute_deps.
> They are passed to subtract_commutative_associative_deps and dereferenced.
 >
> However, this NULL arguments are appropriate for the algorithm used
> in loop_level_carries_dependences. It uses compute_deps
> for finding RAW, WAR and WAW dependences of all basic blocks
> in the body of the given loop. Subsequently, it tries to
> determine presence of these dependences at the given level.
> Therefore it maps the relation of the dependences to the relation
> of the corresponding time-stamps and intersects the result with
> the relation in which all the inputs before the DEPTH occur at the
> same time as the output, and the input at the DEPTH occurs before output.
> If the intersection is not empty, some dependences are carried
> by the DEPTH we currently check and the loop is consequently not parallel.
>
> This patch tries to avoid the problem by addition to
> subtract_commutative_associative_deps
> of NULL checking of the no_source statements.

by adding NULL checking of the no_source statements to subtract_...


> Tested x86_64-unknown-linux-gnu, applying to 4.8.3 and trunk.
>
> 2014-03-07  Roman Gareev  <gareevroman@gmail.com>
>
>                  gcc/
>                  * graphite-dependences.c
> (subtract_commutative_associative_deps):
>                  Add NULL checking of the following variables:
> must_raw_no_source, may_raw_no_source,
>                  must_war_no_source, may_war_no_source,
>                  must_waw_no_source, may_waw_no_source.
>
>                  gcc/testsuite/gfortran.dg/graphite/graphite.exp: Run
> corresponding
>                  tests in new mode.
>
>                  gcc/testsuite/gfortran.dg/graphite/parallelize-all-1.f: New
> testcase.
>
> diff --git a/gcc/graphite-dependences.c b/gcc/graphite-dependences.c
> index b0f8680..f382233 100644
> --- a/gcc/graphite-dependences.c
> +++ b/gcc/graphite-dependences.c
> @@ -426,22 +426,48 @@ subtract_commutative_associative_deps (scop_p scop,
>
>    *must_raw = isl_union_map_subtract (*must_raw, x_must_raw);
>    *may_raw = isl_union_map_subtract (*may_raw, x_may_raw);
> - *must_raw_no_source = isl_union_map_subtract (*must_raw_no_source,
> -      x_must_raw_no_source);
> - *may_raw_no_source = isl_union_map_subtract (*may_raw_no_source,
> -     x_may_raw_no_source);
> +
> + if (must_raw_no_source)
> +  *must_raw_no_source = isl_union_map_subtract (*must_raw_no_source,
> +          x_must_raw_no_source);
> + else
> +  isl_union_map_free (x_must_raw_no_source);
> +
> + if (may_raw_no_source)
> +  *may_raw_no_source = isl_union_map_subtract (*may_raw_no_source,
> +         x_may_raw_no_source);
> + else
> +  isl_union_map_free (x_may_raw_no_source);
> +
>    *must_war = isl_union_map_subtract (*must_war, x_must_war);
>    *may_war = isl_union_map_subtract (*may_war, x_may_war);
> - *must_war_no_source = isl_union_map_subtract (*must_war_no_source,
> -      x_must_war_no_source);
> - *may_war_no_source = isl_union_map_subtract (*may_war_no_source,
> -     x_may_war_no_source);
> +
> + if (must_war_no_source)
> +  *must_war_no_source = isl_union_map_subtract (*must_war_no_source,
> +          x_must_war_no_source);
> + else
> +  isl_union_map_free (x_must_war_no_source);
> +
> + if (may_war_no_source)
> +  *may_war_no_source = isl_union_map_subtract (*may_war_no_source,
> +         x_may_war_no_source);
> + else
> +  isl_union_map_free (x_may_war_no_source);
> +
>    *must_waw = isl_union_map_subtract (*must_waw, x_must_waw);
>    *may_waw = isl_union_map_subtract (*may_waw, x_may_waw);
> - *must_waw_no_source = isl_union_map_subtract (*must_waw_no_source,
> -      x_must_waw_no_source);
> - *may_waw_no_source = isl_union_map_subtract (*may_waw_no_source,
> -     x_may_waw_no_source);
> +
> + if (must_waw_no_source)
> +  *must_waw_no_source = isl_union_map_subtract (*must_waw_no_source,
> +          x_must_waw_no_source);
> + else
> +  isl_union_map_free (x_must_waw_no_source);
> +
> + if (may_waw_no_source)
> +  *may_waw_no_source = isl_union_map_subtract (*may_waw_no_source,
> +         x_may_waw_no_source);
> + else
> +  isl_union_map_free (x_may_waw_no_source);
>         }
>
>     isl_union_map_free (original);
> diff --git a/gcc/testsuite/gfortran.dg/graphite/graphite.exp
> b/gcc/testsuite/gfortran.dg/graphite/graphite.exp
> index c3aad13..3833e43 100644
> --- a/gcc/testsuite/gfortran.dg/graphite/graphite.exp
> +++ b/gcc/testsuite/gfortran.dg/graphite/graphite.exp
> @@ -44,6 +44,7 @@ set interchange_files [lsort [glob -nocomplain
> $srcdir/$subdir/interchange-*.\[f
>   set scop_files        [lsort [glob -nocomplain
> $srcdir/$subdir/scop-*.\[fF\]{,90,95,03,08} ] ]
>   set run_id_files      [lsort [glob -nocomplain
> $srcdir/$subdir/run-id-*.\[fF\]{,90,95,03,08} ] ]
>   set vect_files        [lsort [glob -nocomplain
> $srcdir/$subdir/vect-*.\[fF\]{,90,95,03,08} ] ]
> +set parallelize_files [lsort [glob -nocomplain
> $srcdir/$subdir/parallelize-all-*.\[fF\]{,90,95,03,08} ] ]
>
>   # Tests to be compiled.
>   set dg-do-what-default compile
> @@ -51,6 +52,7 @@ gfortran-dg-runtest $scop_files        "-O2 -fgraphite
> -fdump-tree-graphite-all"
>   gfortran-dg-runtest $id_files          "-O2 -fgraphite-identity
> -ffast-math"
>   gfortran-dg-runtest $interchange_files "-O2 -floop-interchange
> -fno-loop-block -fno-loop-strip-mine -ffast-math -fdump-tree-graphite-all"
>   gfortran-dg-runtest $block_files       "-O2 -floop-block
> -fno-loop-strip-mine -fno-loop-interchange -ffast-math
> -fdump-tree-graphite-all"
> +gfortran-dg-runtest $parallelize_files "-Ofast  -floop-parallelize-all"

I am slightly surprised why this change is necessary. Roman, can you 
shade some light on this change.

>   # Vectorizer tests, to be run or compiled, depending on target
> capabilities.
>   if [check_vect_support_and_set_flags] {
> diff --git a/gcc/testsuite/gfortran.dg/graphite/parallelize-all-1.f
> b/gcc/testsuite/gfortran.dg/graphite/parallelize-all-1.f
> new file mode 100644
> index 0000000..e1156f8
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/graphite/parallelize-all-1.f
> @@ -0,0 +1,9 @@
> +      subroutine subsm ( n, x, xp, xx)
> +      integer        n, m, x(n),xp(n), xx(n), gg(n), dd_p
> +      do 55 i=1, n
> +         dd_p  = dd_p + (x(i) - xx(i))*gg(i)
> + 55   continue
> +      if ( dd_p .gt. 0  ) then
> +         call dcopy( n, xp, 1, x, 1 )
> +      endif
> +      end
>