Re: [graphite] Move to cloog.org interface

[Tobias Grosser <tobias@grosser.es>]

On 08/11/2011 07:16 PM, Sebastian Pop wrote:
> On Thu, Aug 11, 2011 at 13:03, Sebastian Pop<sebpop@gmail.com>  wrote:
>> >  On Thu, Aug 11, 2011 at 12:52, Tobias Grosser<tobias@grosser.es>  wrote:
>>> >>  I will commit this patch after the configure changes are in (and meanwhile
>>> >>  no further improvements were suggested for this patch).
>> >
>> >  Ok, thanks.  Let's hope we will have a configure maintainer that has some
>> >  spare cycles to go over your first 3 patches.
>> >
>> >  I will post my patches that convert graphite to ISL on top of your patches.
> I am following the PET (Polyhedral Extraction Tool) git://repo.or.cz/pet.git
> code as suggested by Tobias and Sven in order to help with the translation
> of graphite to ISL.
>
> Here is where I am right now: I am building the ISL counterpart for
> scop->context
> pbb->domain
> pdr->accesses
> and I still have to translate to ISL the original and transformed scattering.
>
> The plan is to build the ISL representation in parallel with PPL data structs,
> then make sure that ISL sets and maps contain valid data, and then move
> away from PPL data structures one by one.
>
> I would appreciate preliminary remarks on these patches.

Hey,

wonderful. Here my first remarks:

> 0005-Remove-ATTRIBUTE_UNUSED.patch
No comment.

> 0006-Add-ISL-data-structures.patch
> 0007-Add-scop-context.patch
>   /* Compute the lower bound LOW and upper bound UP for the induction
> @@ -1360,10 +1368,32 @@ build_cloog_prog (scop_p scop, CloogProgram *prog,

This needs to be adapted to my cloog.org interface patch.

> +/* Prints an isl_set S to stderr.  */
> +
> +DEBUG_FUNCTION void
> +debug_isl_set (isl_set *s)
> +{
> +  isl_ctx *ctx = isl_ctx_alloc ();
> +  isl_printer *pp = isl_printer_to_file (ctx, stderr);
> +  pp = isl_printer_print_set (pp, s);
> +  isl_printer_free (pp);
> +  isl_ctx_free (ctx);
> +}
> +
> +/* Prints an isl_pw_aff A to stderr.  */
> +
> +DEBUG_FUNCTION void
> +debug_isl_pwaff (isl_pw_aff *a)
> +{
> +  isl_ctx *ctx = isl_ctx_alloc ();
> +  isl_printer *pp = isl_printer_to_file (ctx, stderr);
> +  pp = isl_printer_print_pw_aff (pp, a);
> +  isl_printer_free (pp);
> +  isl_ctx_free (ctx);
> +}
> +
> +/* Prints an isl_aff A to stderr.  */
> +
> +DEBUG_FUNCTION void
> +debug_isl_aff (isl_aff *a)
> +{
> +  isl_ctx *ctx = isl_ctx_alloc ();
> +  isl_printer *pp = isl_printer_to_file (ctx, stderr);
> +  pp = isl_printer_print_aff (pp, a);
> +  isl_printer_free (pp);
> +  isl_ctx_free (ctx);
> +}

No need to define these. isl_set_dump(isl_set *) should do what you 
want. Similar functions are defined for other data types.
Furthermore, gdb should also automatically an islprint method, that
allows you to dump (almost) any isl type.

> +static inline bool
> +isl_set_is_equal_ppl_polyhedron (isl_set *s1, ppl_const_Polyhedron_t ph,
> +				 int nb_params, CloogState *state)
> +{
> +  isl_set *s2 = isl_set_from_cloog_domain
> +    (new_Cloog_Domain_from_ppl_Polyhedron (ph, nb_params, state));
> +  int res = isl_set_is_equal (s1, s2);
> +  isl_set_free (s2);
> +  return res;
> +}
Clever. ;-)

> +/*
> +  gcc_assert (isl_set_is_equal_ppl_powerset (scop->context,
> +					     SCOP_CONTEXT (scop),
> +					     scop_nb_params (scop),
> +					     cloog_state));
> +*/
Seems to be useless.

>   #endif /* GRAPHITE_CLOOG_UTIL_H */
> diff --git a/gcc/graphite-poly.c b/gcc/graphite-poly.c
> index af40d20..225c8a2 100644
> --- a/gcc/graphite-poly.c
> +++ b/gcc/graphite-poly.c
> @@ -1012,6 +1012,7 @@ new_scop (void *region)
>     scop_p scop = XNEW (struct scop);
>
>     SCOP_CONTEXT (scop) = NULL;
> +  scop->context = NULL;
>     scop_set_region (scop, region);
>     SCOP_BBS (scop) = VEC_alloc (poly_bb_p, heap, 3);
>     SCOP_ORIGINAL_PDDRS (scop) = htab_create (10, hash_poly_ddr_p,
> @@ -1040,6 +1041,9 @@ free_scop (scop_p scop)
>     if (SCOP_CONTEXT (scop))
>       ppl_delete_Pointset_Powerset_C_Polyhedron (SCOP_CONTEXT (scop));
>
> +  if (scop->context)
> +    isl_set_free (scop->context);
The if is most probably not needed. Sven recently stated, that passing 
NULL to
isl_*_free functions has defined semantics.

> diff --git a/gcc/graphite-poly.h b/gcc/graphite-poly.h
> index 3a5fddb..775c853 100644
> --- a/gcc/graphite-poly.h
> +++ b/gcc/graphite-poly.h
> @@ -1409,6 +1409,9 @@ struct scop
>     ppl_Pointset_Powerset_C_Polyhedron_t _context;
>     isl_set *context;
>
> +  /* The context used internally by ISL.  */
> +  isl_ctx *ctx;
> +
>     /* A hashtable of the data dependence relations for the original
>        scattering.  */
>     htab_t original_pddrs;

> +/* Return an ISL identifier from the name of the ssa_name E.  */
> +
> +static isl_id *
> +isl_id_for_ssa_name (scop_p s, tree e)
> +{
> +  const char *name = get_name (e);
> +  isl_id *id;
> +
> +  if (name)
> +    id = isl_id_alloc (s->ctx, name, e);
Does get_name() return always a unique name or is just the tuple 
(get_name(e), SSA_NAME_VERSION(e)) unique?

> +/* Return an ISL identifier from the name of the ssa_name E.  */
> +
> +static isl_id *
> +isl_id_for_loop (scop_p s, loop_p l)

The comment for this function does not match.

> +/* Compute pwaff mod 2^width.  */
> +
> +static isl_pw_aff *
> +wrap (isl_pw_aff *pwaff, unsigned width)
> +{
> +  isl_int mod;
> +
> +  isl_int_init (mod);
> +  isl_int_set_si (mod, 1);
> +  isl_int_mul_2exp (mod, mod, width);
> +
> +  pwaff = isl_pw_aff_mod (pwaff, mod);
> +
> +  isl_int_clear (mod);
> +
> +  return pwaff;
> +}

You may just want to use isl_pw_aff_mod().
> diff --git a/gcc/graphite.c b/gcc/graphite.c
> index 8f6d8a1..b2cf7c6 100644
> --- a/gcc/graphite.c
> +++ b/gcc/graphite.c
> @@ -260,10 +260,12 @@ graphite_transform_loops (void)
>     bool need_cfg_cleanup_p = false;
>     VEC (scop_p, heap) *scops = NULL;
>     htab_t bb_pbb_mapping;
> +  isl_ctx *ctx;
>
>     if (!graphite_initialize ())
>       return;t
>
> +  ctx = isl_ctx_alloc ();
>     build_scops (&scops);
>
>     if (dump_file&&  (dump_flags&  TDF_DETAILS))
> @@ -277,6 +279,7 @@ graphite_transform_loops (void)
>     FOR_EACH_VEC_ELT (scop_p, scops, i, scop)
>       if (dbg_cnt (graphite_scop))
>         {
> +	scop->ctx = ctx;
>   	build_poly_scop (scop);
>
>   	if (POLY_SCOP_P (scop)
> @@ -288,6 +291,7 @@ graphite_transform_loops (void)
>     htab_delete (bb_pbb_mapping);
>     free_scops (scops);
>     graphite_finalize (need_cfg_cleanup_p);
> +  isl_ctx_free (ctx);

We should check with Sven if there will be any troubles/problems, 
because ClooG is allocating its own context and we are passing 
isl_objects between those two contexts.

I think the best would be to provide our ctx to cloog when allocating
the CloogState.

> 0008-fix-memory-leak.patch
No comment.

> 0009-add-pbb-domain.patch
> diff --git a/gcc/graphite-poly.c b/gcc/graphite-poly.c
> index 225c8a2..c5b32d6 100644
> --- a/gcc/graphite-poly.c
> +++ b/gcc/graphite-poly.c
> @@ -901,7 +902,11 @@ free_poly_bb (poly_bb_p pbb)
>     int i;
>     poly_dr_p pdr;
>
> -  ppl_delete_Pointset_Powerset_C_Polyhedron (PBB_DOMAIN (pbb));
> +  if (PBB_DOMAIN (pbb))
> +    ppl_delete_Pointset_Powerset_C_Polyhedron (PBB_DOMAIN (pbb));
> +
> +  if (pbb->domain)
> +    isl_set_free (pbb->domain);
The if is not needed here.

>     if (PBB_TRANSFORMED (pbb))
>       poly_scattering_free (PBB_TRANSFORMED (pbb));
> @@ -1060,6 +1065,9 @@ openscop_print_pbb_domain (FILE *file, poly_bb_p pbb, int verbosity)
>     graphite_dim_t i;
>     gimple_bb_p gbb = PBB_BLACK_BOX (pbb);
>
> +  if (isl_set_plain_is_empty (pbb->domain))
> +    return;

Why do we return if a domain is empty. There may be cases where the 
domain is empty because some constraints show us that the code will 
never be executed. Still, I think it is good to show that this PBB
has a valid, though empty, domain.

> 0010-add-pdr-accesses-and-pdr-extent.patch
> +/* Prints an isl_map M to stderr.  */
> +
> +DEBUG_FUNCTION void
> +debug_isl_map (isl_map *m)
> +{
> +  isl_ctx *ctx = isl_ctx_alloc ();
> +  isl_printer *pp = isl_printer_to_file (ctx, stderr);
> +  pp = isl_printer_print_map (pp, m);
> +  isl_printer_free (pp);
> +  isl_ctx_free (ctx);
> +}
> +

Not needed. Use isl_map_dump(isl_map*).

> +DEBUG_FUNCTION void
> +debug_isl_id (isl_id *i)
> +{
> +  isl_ctx *ctx = isl_ctx_alloc ();
> +  isl_printer *pp = isl_printer_to_file (ctx, stderr);
> +  pp = isl_printer_print_id (pp, i);
> +  isl_printer_free (pp);
> +  isl_ctx_free (ctx);
> +}
Does isl also have a dump function for this?

That's from my side. I am extremely impressed by your progress, and 
believe this move makes the code of graphite both a lot more readable 
and hopefully also a lot more correct.

Cheers and thanks for all the work!
Tobi