diff mbox

[Graphite] Fix PR50561

Message ID alpine.LNX.2.00.1201091629520.4999@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Jan. 9, 2012, 3:31 p.m. UTC
We have two Graphite related release blockers for 4.7.  The
following patch fixes the first one.

The asserts in

static inline ppl_dimension_type
psct_dynamic_dim (poly_bb_p pbb, graphite_dim_t level)
{
  graphite_dim_t result = 1 + 2 * level;

  gcc_assert (result < pbb_nb_scattering_transform (pbb));
  return result;
}

static inline ppl_dimension_type
psct_static_dim (poly_bb_p pbb, graphite_dim_t level)
{
  graphite_dim_t result = 2 * level;

  gcc_assert (result < pbb_nb_scattering_transform (pbb));
  return result;
}

are not consistent in case both a dynamic and static dimension should
always exist at the same time.  Changing psct_dynamic_dim to use <=
fixes the issue.

Tobias, you are the only active(?) Graphite reviewer left.

Ok?

Thanks,
Richard.

2012-01-09  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/50561
	* graphite-poly.h (psct_dynamic_dim): Fix assert.

	* gcc.dg/graphite/pr50561.c: New testcase.

Comments

Tobias Grosser Jan. 9, 2012, 10:22 p.m. UTC | #1
On 01/09/2012 04:31 PM, Richard Guenther wrote:
>
> We have two Graphite related release blockers for 4.7.  The
> following patch fixes the first one.

Hi Richard,

thanks a lot for working on this.

> The asserts in
>
> static inline ppl_dimension_type
> psct_dynamic_dim (poly_bb_p pbb, graphite_dim_t level)
> {
>    graphite_dim_t result = 1 + 2 * level;
>
>    gcc_assert (result<  pbb_nb_scattering_transform (pbb));
>    return result;
> }
>
> static inline ppl_dimension_type
> psct_static_dim (poly_bb_p pbb, graphite_dim_t level)
> {
>    graphite_dim_t result = 2 * level;
>
>    gcc_assert (result<  pbb_nb_scattering_transform (pbb));
>    return result;
> }
>
> are not consistent in case both a dynamic and static dimension should
> always exist at the same time.  Changing psct_dynamic_dim to use<=
> fixes the issue.

I don't think this is the right fix. The asserts seem to be correct.
The problem is actually already shown when you run cc1.

[CLooG] INFO: 2 dimensions (over 3) are scalar.
[CLooG] WARNING: not enough constraints for 1 scattering function(s).

This means, the schedule/scattering that we defined was not sufficient
enough to define the execution order. It is underspecified. The result 
is that CLooG generates the following code:

for (c2=-250;c2<=1535;c2++) {
   for (i=max(0,ceild(51*c2,307))
        i<=min(255,floord(51*c2+12800,307));i++) {
     S1(i);
   }
}

c2 is a scattering variable, but 'i' is actually a original loop 
dimension. I believe graphite does not expect this dimension to show up.
Somehow the scattering that is defined is incorrect. I suspect the loop 
flattening has some problems with the inequalities introduced by the 
strip-mining.

I will look into this.

> Tobias, you are the only active(?) Graphite reviewer left.

I am, though i have currently not a lot of time to write patches myself.

I am still looking into finishing Sebastians patches. Afterwards it 
should be simple to use the new polyhedral optimizer as it was recently 
added to isl.

Tobi
diff mbox

Patch

Index: gcc/graphite-poly.h
===================================================================
--- gcc/graphite-poly.h	(revision 183013)
+++ gcc/graphite-poly.h	(working copy)
@@ -656,7 +656,7 @@  psct_dynamic_dim (poly_bb_p pbb, graphit
 {
   graphite_dim_t result = 1 + 2 * level;
 
-  gcc_assert (result < pbb_nb_scattering_transform (pbb));
+  gcc_assert (result <= pbb_nb_scattering_transform (pbb));
   return result;
 }
 
Index: gcc/testsuite/gcc.dg/graphite/pr50561.c
===================================================================
--- gcc/testsuite/gcc.dg/graphite/pr50561.c	(revision 0)
+++ gcc/testsuite/gcc.dg/graphite/pr50561.c	(revision 0)
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -floop-flatten -floop-strip-mine" } */
+
+void f (unsigned *s)
+{
+  int n;
+  for (n = 0; n < 256; n++)
+    s[n] = 0;
+}