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

[Tobias Grosser <tobias at grosser dot es>]

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 Gareev<gareevroman@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::map<isl_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;
> +}