[GSoC] How to get started with the isl code generation

[Tobias Grosser]

On 08/06/2014 19:43, Roman Gareev wrote:
> Hi Tobias,
>
>> This file is empty. It seems to be the perfect place for gloog_isl,
>> maybe give it a more descriptive name. E.g.,
>>
>> graphite_regenerate_ast_isl()
>>
>> We could then rename gloog, to graphite_regenerate_ast_cloog().
>>
>> gloog comes from graphite + cloog and does not make sense in the context
>> of isl.
>
> Would it be better to rename gloog in a separate patch (because it
> could cause renaming of gloog_error, for eaxmple, and increase the
> size of the patch)?

Yes, you are right. Please make this a separate patch and rebase this
patch on the gloog patch.

> I tried to incorporate all your comments in the following patch. I
> also attach the change log to this message.

Comments inline.

>  #endif /* ! GCC_FLAG_TYPES_H */
> diff --git a/gcc/graphite-clast-to-gimple.h b/gcc/graphite-clast-to-gimple.h
> index fc5a679..5e23d94 100644
> --- a/gcc/graphite-clast-to-gimple.h
> +++ b/gcc/graphite-clast-to-gimple.h
> @@ -21,6 +21,8 @@ along with GCC; see the file COPYING3.  If not see
>  #ifndef GCC_GRAPHITE_CLAST_TO_GIMPLE_H
>  #define GCC_GRAPHITE_CLAST_TO_GIMPLE_H
>
> +#include "graphite-htab.h"
> +
>  extern CloogState *cloog_state;
>
>  /* Data structure for CLooG program representation.  */
> @@ -30,14 +32,7 @@ struct cloog_prog_clast {
>    struct clast_stmt *stmt;
>  };
>
> -/* Stores BB's related PBB.  */
> -
> -struct bb_pbb_def
> -{
> -  basic_block bb;
> -  poly_bb_p pbb;
> -};
> -
> +extern bool gloog (scop_p, bb_pbb_htab_type);
>  extern void debug_clast_stmt (struct clast_stmt *);
>  extern void print_clast_stmt (FILE *, struct clast_stmt *);
>
> diff --git a/gcc/graphite-htab.h b/gcc/graphite-htab.h
> index d67dd0c..9f31fac 100644
> --- a/gcc/graphite-htab.h
> +++ b/gcc/graphite-htab.h
> @@ -22,7 +22,14 @@ along with GCC; see the file COPYING3.  If not see
>  #define GCC_GRAPHITE_HTAB_H
>
>  #include "hash-table.h"
> -#include "graphite-clast-to-gimple.h"
> +
> +/* Stores BB's related PBB.  */
> +
> +struct bb_pbb_def
> +{
> +  basic_block bb;
> +  poly_bb_p pbb;
> +};
>
>  /* Hashtable helpers.  */
>
> @@ -52,7 +59,6 @@ bb_pbb_hasher::equal (const value_type *bp1, const compare_type *bp2)
>
>  typedef hash_table <bb_pbb_hasher> bb_pbb_htab_type;
>
> -extern bool gloog (scop_p, bb_pbb_htab_type);
>  poly_bb_p find_pbb_via_hash (bb_pbb_htab_type, basic_block);
>  bool loop_is_parallel_p (loop_p, bb_pbb_htab_type, int);
>  scop_p get_loop_body_pbbs (loop_p, bb_pbb_htab_type, vec<poly_bb_p> *);

I assume all this could be part of the gloog patch?

> diff --git a/gcc/graphite-isl-ast-to-gimple.c b/gcc/graphite-isl-ast-to-gimple.c
> new file mode 100644
> index 0000000..309ba28
> --- /dev/null
> +++ b/gcc/graphite-isl-ast-to-gimple.c
> @@ -0,0 +1,132 @@
> +/* Translation of ISL AST to Gimple.
> +   Copyright (C) 2014 Free Software Foundation, Inc.
> +   Contributed by Roman Gareev <gareevroman@gmail.com>.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 3, or (at your option)
> +any later version.
> +
> +GCC is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3.  If not see
> +<http://www.gnu.org/licenses/>.  */
> +
> +#include "config.h"
> +
> +#include <isl/set.h>
> +#include <isl/map.h>
> +#include <isl/union_map.h>
> +#include <isl/ast_build.h>
> +
> +#include "system.h"
> +#include "coretypes.h"
> +#include "tree.h"
> +#include "basic-block.h"
> +#include "tree-ssa-alias.h"
> +#include "internal-fn.h"
> +#include "gimple-expr.h"
> +#include "is-a.h"
> +#include "gimple.h"
> +#include "gimple-iterator.h"
> +#include "tree-ssa-loop.h"
> +#include "tree-pass.h"
> +#include "cfgloop.h"
> +#include "tree-data-ref.h"
> +#include "sese.h"

I am surprised. Are all these includes really needed to get _this_ patch
compile? (I asked this before).

> diff --git a/gcc/testsuite/gcc.dg/graphite/scop-23.c b/gcc/testsuite/gcc.dg/graphite/scop-23.c
> new file mode 100644
> index 0000000..868222a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/graphite/scop-23.c

Maybe a more descriptive name:

isl-codegen-loop-dumping.c

> @@ -0,0 +1,32 @@
> +/* { dg-additional-options "-floop-parallelize-all -O2 -fgraphite-code-generator=isl" } */

Why do we need -floop-parallelize-all? -fgraphite-identity should be
enough.

> +#include <stdio.h>
> +
> +#define N 16
No need for these defines.
> +
> +int
> +main1 (int n, int *a)
> +{
> +  int i, j;
> +
> +  for (i = 0; i < n - 1; i++)
> +    for (j = 0; j < n; j++)
> +      a[j] = i + n;
And just a single loop.

> +
> +  for (j = 0; j < n; j++)
> +    if (a[j] != i + n - 1)
> +      __builtin_abort ();
No need for the check.

> +int
> +main ()
> +{
> +  int a[N];
> +  main1 (N, a);
> +  return 0;
> +}
No need for main.

And we have a minimal test case.

> +/* { dg-final { scan-tree-dump-times "ISL AST generated by ISL: \nfor \\(int c1 = 0; c1 < n - 1; c1 \\+= 1\\)\n  for \\(int c3 = 0; c3 < n; c3 \\+= 1\\)\n    S_5\\(c1, c3\\);" 1 "graphite"} } */

This is ugly, but there is little we can do about it. Maybe you can ask
on the mailing list if there is a way to write this in multiple lines?

On possibility might be that you write:

> +/* { dg-final { scan-tree-dump-times "ISL AST generated by ISL: \n
? for \\(int c1 = 0; c1 < n - 1; c1 \\+= 1\\)\n
?  for \\(int c3 = 0; c3 < n; c3 \\+= 1\\)\n
?    S_5\\(c1, c3\\);" 1 "graphite"}
? } */

The idea is that you add newlines, but the question mark that is
following immediately after makes them again optional in the regexp.

I would spend maybe 30-45 minutes on this. If you don't find any
solution, let's live with it.

Cheers,
Tobias