Re: [GSoC] A bug related to induction variables and blocks

2014-07-24 Thread Roman Gareev
 Is there a reason you have those global values? To my understanding they
 could possibly just be function parameters?

Yes, it would be fine for this test case. I've implemented this in the
improved version.

--
   Cheers, Roman Gareev.
2014-07-23  Roman Gareev  gareevro...@gmail.com

[gcc/]

* graphite-isl-ast-to-gimple.c:
(graphite_create_new_loop): Add calling of isl_id_free to properly
decrement reference counts.

[gcc/testsuite]

* gcc.dg/graphite/isl-ast-gen-blocks-4.c: New testcase.
Index: gcc/graphite-isl-ast-to-gimple.c
===
--- gcc/graphite-isl-ast-to-gimple.c(revision 212922)
+++ gcc/graphite-isl-ast-to-gimple.c(working copy)
@@ -383,6 +383,10 @@
 
   isl_ast_expr *for_iterator = isl_ast_node_for_get_iterator (node_for);
   isl_id *id = isl_ast_expr_get_id (for_iterator);
+  std::mapisl_id *, tree::iterator res;
+  res = ip.find (id);
+  if (ip.count (id))
+isl_id_free (res-first);
   ip[id] = iv;
   isl_ast_expr_free (for_iterator);
   return loop;
Index: gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c
===
--- gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c(revision 0)
+++ gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c(working copy)
@@ -0,0 +1,32 @@
+/* { dg-do run } */
+/* { dg-options -O2 -fgraphite-identity -fgraphite-code-generator=isl } */
+
+static int __attribute__((noinline))
+foo (int k, int n1, int n2, int n3)
+{
+  int j, res = 0;
+  for (j = 0; j  k; j++)
+{
+  int i;
+  for (i = 0; i  n1; i++)
+res += i;
+  for (i = 0; i  n2; i++)
+res += i;
+  for (i = 0; i  n3; i++)
+res += i;
+}
+
+  return res;
+}
+
+extern void abort ();
+
+int
+main (void)
+{ 
+  int res = foo (4, 50, 50, 50);
+  if (res != 14700)
+abort ();
+
+  return 0;
+}


Re: [GSoC] A bug related to induction variables and blocks

2014-07-24 Thread Tobias Grosser

On 24/07/2014 12:09, Roman Gareev wrote:

Is there a reason you have those global values? To my understanding they
could possibly just be function parameters?


Yes, it would be fine for this test case. I've implemented this in the
improved version.


LGTM.

Tobias



[GSoC] A bug related to induction variables and blocks

2014-07-23 Thread Roman Gareev
If we try to generate code from the following ISL AST:

{
  for (int c1 = 0; c1  k.3; c1 += 1) {
S_21(c1);
S_21(c1);
S_4(c1);
for (int c3 = 0; c3  pretmp; c3 += 1)
  S_5(c1, c3);
S_7(c1);
S_26(c1);
S_8(c1);
S_8(c1);
S_9(c1);
for (int c3 = 0; c3  pretmp; c3 += 1)
  S_10(c1, c3);
S_11(c1);
S_25(c1);
S_13(c1);
S_13(c1);
S_14(c1);
for (int c3 = 0; c3  pretmp; c3 += 1)
  S_15(c1, c3);
S_16(c1);
S_24(c1);
S_18(c1);
  }
  S_19();
}

we'll get the following error:

isl_ctx.c:172: isl_ctx freed, but some objects still reference it

If I'm not mistaken, it is caused by missing of isl_id_free functions,
which should be called in the same quantity as isl_ast_expr_get_id
because of the implementation of ISL.

I've attached the patch, which may fix the problem. It also contains a
corresponding test case. Is it fine for trunk?


--
   Cheers, Roman Gareev.
2014-07-23  Roman Gareev  gareevro...@gmail.com

[gcc/]

* graphite-isl-ast-to-gimple.c:
(graphite_create_new_loop): Add calling of isl_id_free.

[gcc/testsuite]

* gcc.dg/graphite/isl-ast-gen-blocks-4.c: New testcase.
Index: gcc/graphite-isl-ast-to-gimple.c
===
--- gcc/graphite-isl-ast-to-gimple.c(revision 212922)
+++ gcc/graphite-isl-ast-to-gimple.c(working copy)
@@ -383,6 +383,10 @@
 
   isl_ast_expr *for_iterator = isl_ast_node_for_get_iterator (node_for);
   isl_id *id = isl_ast_expr_get_id (for_iterator);
+  std::mapisl_id *, tree::iterator res;
+  res = ip.find (id);
+  if (res != ip.end ())
+isl_id_free (res-first);
   ip[id] = iv;
   isl_ast_expr_free (for_iterator);
   return loop;

Index: gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c
===
--- gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c(revision 0)
+++ gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c(working copy)
@@ -0,0 +1,37 @@
+/* { dg-do run } */
+/* { dg-options -O2 -fgraphite-identity -fgraphite-code-generator=isl } */
+
+int k = 4;
+int n1 = 50;
+int n2 = 50;
+int n3 = 50;
+
+static int __attribute__((noinline))
+foo ()
+{
+  int j, res = 0;
+  for (j = 0, res = 0; j  k; j++)
+{
+  int i;
+  for (i = 0; i  n1; i++)
+res += i;
+  for (i = 0; i  n2; i++)
+res += i;
+  for (i = 0; i  n3; i++)
+res += i;
+}
+
+  return res;
+}
+
+extern void abort ();
+
+int
+main (void)
+{ 
+  int res = foo ();
+  if (res != 14700)
+abort ();
+
+  return 0;
+}


Re: [GSoC] A bug related to induction variables and blocks

2014-07-23 Thread Tobias Grosser

On 23/07/2014 16:55, Roman Gareev wrote:

If we try to generate code from the following ISL AST:

{
   for (int c1 = 0; c1  k.3; c1 += 1) {
 S_21(c1);
 S_21(c1);
 S_4(c1);
 for (int c3 = 0; c3  pretmp; c3 += 1)
   S_5(c1, c3);
 S_7(c1);
 S_26(c1);
 S_8(c1);
 S_8(c1);
 S_9(c1);
 for (int c3 = 0; c3  pretmp; c3 += 1)
   S_10(c1, c3);
 S_11(c1);
 S_25(c1);
 S_13(c1);
 S_13(c1);
 S_14(c1);
 for (int c3 = 0; c3  pretmp; c3 += 1)
   S_15(c1, c3);
 S_16(c1);
 S_24(c1);
 S_18(c1);
   }
   S_19();
}

we'll get the following error:

isl_ctx.c:172: isl_ctx freed, but some objects still reference it

If I'm not mistaken, it is caused by missing of isl_id_free functions,
which should be called in the same quantity as isl_ast_expr_get_id
because of the implementation of ISL.

I've attached the patch, which may fix the problem. It also contains a
corresponding test case. Is it fine for trunk?


LGTM. Some minor comments.


ChangeLog_entry.txt


2014-07-23  Roman Gareevgareevro...@gmail.com

[gcc/]

* graphite-isl-ast-to-gimple.c:
(graphite_create_new_loop): Add calling of isl_id_free.


Add calling of isl_id_free to properly decrement reference counts.



[gcc/testsuite]

* gcc.dg/graphite/isl-ast-gen-blocks-4.c: New testcase.


patch.txt


Index: gcc/graphite-isl-ast-to-gimple.c
===
--- gcc/graphite-isl-ast-to-gimple.c(revision 212922)
+++ gcc/graphite-isl-ast-to-gimple.c(working copy)
@@ -383,6 +383,10 @@

isl_ast_expr *for_iterator = isl_ast_node_for_get_iterator (node_for);
isl_id *id = isl_ast_expr_get_id (for_iterator);
+  std::mapisl_id *, tree::iterator res;
+  res = ip.find (id);
+  if (res != ip.end ())


What about:

if (ip.count(id))
  isl_id_free(res-first())


+isl_id_free (res-first);
ip[id] = iv;
isl_ast_expr_free (for_iterator);
return loop;

Index: gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c
===
--- gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c(revision 0)
+++ gcc/testsuite/gcc.dg/graphite/isl-ast-gen-blocks-4.c(working copy)
@@ -0,0 +1,37 @@
+/* { dg-do run } */
+/* { dg-options -O2 -fgraphite-identity -fgraphite-code-generator=isl } */
+
+int k = 4;
+int n1 = 50;
+int n2 = 50;
+int n3 = 50;


Is there a reason you have those global values? To my understanding they 
could possibly just be function parameters?



+static int __attribute__((noinline))
+foo ()
+{
+  int j, res = 0;
+  for (j = 0, res = 0; j  k; j++)
+{
+  int i;
+  for (i = 0; i  n1; i++)
+res += i;
+  for (i = 0; i  n2; i++)
+res += i;
+  for (i = 0; i  n3; i++)
+res += i;
+}
+
+  return res;
+}
+
+extern void abort ();
+
+int
+main (void)
+{
+  int res = foo ();
+  if (res != 14700)
+abort ();
+
+  return 0;
+}