Re: isl scheduler and spatial locality (Re: [PATCH][GRAPHITE] More TLC)

2017-11-11 Thread Sven Verdoolaege
On Sun, Oct 01, 2017 at 11:58:30AM +0200, Sven Verdoolaege wrote:
> For the approach pluto is taking, you'll have to look at the source
> code, see pluto_intra_tile_optimize_band.
> For the other two approaches I mentioned above, reports will
> be made available within the next couple of weeks.

https://hal.inria.fr/hal-01628798
http://www.cs.kuleuven.be/publicaties/rapporten/cw/CW709.abs.html

skimo


Re: [GRAPHITE, PATCH] Loop unroll and jam optimization

2014-11-08 Thread Sven Verdoolaege
On Sat, Nov 08, 2014 at 12:32:05AM +0100, Mircea Namolaru wrote:
> Hello, 
> 
> This is the patch for unroll and jam optimizations. It is based on the
> code written by Tobias from graphite-optimize-isl.c (the code was
> unreachable till now) extended and enabled it via a new option 
> -floop-unroll-jam.
> The patch takes advantage of the new isl based code generator introduced 
> recently 
> in GCC (in fact of the possible options for building the AST).
> 
> The code generated for this optimization in the case of non-constant loop 
> bounds
> initially looks as below. This is not very useful because the standard GCC 
> unrolling don't succeed to unroll the most inner loop.
> 
> ISL AST generated by ISL: 
> for (int c0 = 0; c0 < HEIGHT; c0 += 4)
>   for (int c1 = 0; c1 < LENGTH - 3; c1 += 1)
> for (int c2 = c0; c2 <= min(HEIGHT - 1, c0 + 3); c2 += 1)
>   S_4(c2, c1);
> 
> Now, the "separating class" option (set for unroll and jam) produces this 
> nice loop 
> structure: 

I'm not sure if Tobi or Albert have told you, but the separation_class option
is going to be phased out since its design is fundamentally flawed.
If you can't wait until isl-0.15, then I guess you have no choice but
to use this option, but you should realize that it will remain frozen
in its current broken state (until it is removed at some point).

> +
> +/* Set the unroll AST built option.  */
> +
> +static __isl_give isl_union_map *
> +generate_isl_options_2 (scop_p scop, __isl_take isl_union_set *domain, 
> + int dim, int cl)

Not a very descriptive function name.

> +{
> +  isl_map  *map;
> +  isl_space *space, *space_sep;
> +  isl_ctx *ctx;
> +  isl_union_map *mapu;
> +  int nsched = get_max_schedule_dimensions (scop);
> + 
> +  ctx = scop->ctx;
> +  space_sep = isl_space_alloc(ctx, 0, 1, 1);
> +  space_sep = isl_space_wrap(space_sep);
> +  space_sep = isl_space_set_tuple_name(space_sep, isl_dim_set,
> +"separation_class");
> +  space = isl_set_get_space (scop->context);
> +  space_sep = isl_space_align_params(space_sep, isl_space_copy(space));
> +  space = isl_space_map_from_domain_and_range(space, space_sep);
> +  space = isl_space_add_dims (space,isl_dim_in, nsched);

Inconsistent spacing, sometimes with a space before "(" and sometimes
without.
I also noticed some tab/spacing inconsistencies later in the patch,
but I already removed that part in my reply.

> +  /* Extract the original and auxiliar maps from pbb->transformed.
> + Set pbb->transformed to the original map. */
> +  psmap = &smap;
> +  psmap->n = 0;
> +  res = isl_map_foreach_basic_map (pbb->transformed, separate_map, (void 
> *)psmap);
> +  gcc_assert (res == 0);
> +
> +  isl_map_free(pbb->transformed);
> +  pbb->transformed = isl_map_copy(psmap->map_arr[0]);
> +

I have no idea what this pbb->transformed is supposed to represent,
but you appear to be assuming that it has exactly two disjuncts and that
they appear in some order.  Now, perhaps you have explicitly
checked that this map has two disjuncts, but then you should
probably bring the check closer since any operation on sets that
you perform could change the internal representation (even of
other sets).  However, in no way can you assume that
isl_map_foreach_basic_map will iterate over these disjuncts
in any specific order.

> +  bb_domain_s = isl_set_apply (isl_set_copy (bb_domain),
> +psmap->map_arr[1]);
> +  bb_domain_r = isl_set_apply (bb_domain, psmap->map_arr[0]);  
> +
> +  bb_domain_s = isl_set_complement (bb_domain_s);
> +  bb_domain_r = isl_set_subtract(bb_domain_r,bb_domain_s);

Why are you subtracting the complement of a set instead of just
intersecting with that set?

> +  /* Create an identity map for everything except DimToVectorize and the 
> + point loop. */
> +  for (i = 0; i < ScheduleDimensions; i++)
> +{
> +  if (i == DimToVectorize)
> +continue;
> +
> +  c = isl_equality_alloc (isl_local_space_copy (LocalSpace));
> +
> +  isl_constraint_set_coefficient_si (c, isl_dim_in, i, -1);
> +  isl_constraint_set_coefficient_si (c, isl_dim_out, i, 1);
> +
> +  TilingMap = isl_map_add_constraint (TilingMap, c);

You can use isl_map_equate instead.

> @@ -350,17 +425,31 @@
> SuffixSchedule);
> isl_band_list_free (Children);

Gaack!  gcc is using band forests too...
I guess I'll have to keep them around for a while then

skimo


Re: [GRAPHITE, PATCH] Loop unroll and jam optimization

2014-11-11 Thread Sven Verdoolaege
On Tue, Nov 11, 2014 at 04:05:57PM +0100, Mircea Namolaru wrote:
> > I'm not sure if Tobi or Albert have told you, but the separation_class 
> > option
> > is going to be phased out since its design is fundamentally flawed.
> > If you can't wait until isl-0.15, then I guess you have no choice but
> > to use this option, but you should realize that it will remain frozen
> > in its current broken state (until it is removed at some point).
> > 
> 
> No, didn't know about the phase out of separation_class option. Anyway, for 
> the time 
> being is the best solution available. My understanding is that this option 
> should
> always generate correct code, of course as long as the scheduling is correct, 
> but
> think that had some cases when setting the separating_class leads to 
> incorrect code. 
> 
> For isl_0.15, do you intend to provide some option with a similar 
> functionality ? 

Yes.

> > > +  /* Extract the original and auxiliar maps from pbb->transformed.
> > > + Set pbb->transformed to the original map. */
> > > +  psmap = &smap;
> > > +  psmap->n = 0;
> > > +  res = isl_map_foreach_basic_map (pbb->transformed, separate_map,
> > > (void *)psmap);
> > > +  gcc_assert (res == 0);
> > > +
> > > +  isl_map_free(pbb->transformed);
> > > +  pbb->transformed = isl_map_copy(psmap->map_arr[0]);
> > > +
> > 
> > I have no idea what this pbb->transformed is supposed to represent,
> > but you appear to be assuming that it has exactly two disjuncts and that
> > they appear in some order.  Now, perhaps you have explicitly
> > checked that this map has two disjuncts, but then you should
> > probably bring the check closer since any operation on sets that
> > you perform could change the internal representation (even of
> > other sets).  However, in no way can you assume that
> > isl_map_foreach_basic_map will iterate over these disjuncts
> > in any specific order.
> >
> 
> At this point pbb->transformed has two basic maps, one is the mapping for 
> unroll and jam, 
> and one for the full tile for the striped dimension. Introduce a check that 
> differentiate 
> between them as the image of one maps should be included in the other.

I didn't do a full review (and I won't have time for it either),
but at a quick glance,
you still seem to be assuming that if you take the union of two
basic maps, that you can extract the original two basic maps from the union.
In general, you can't.  At least you shouldn't assume that you can.
Besides, your updated code is also pretty convoluted,
with very little documentation.

skimo


Re: [PATCH][GRAPHITE] More TLC

2017-09-26 Thread Sven Verdoolaege
On Tue, Sep 26, 2017 at 09:19:50AM -0500, Sebastian Pop wrote:
> Sven, is there already a function that computes the sum of all
> strides in a proximity map?  Maybe you have code that does
> something similar in pet or ppcg?

What exactly do you want to sum?
If this involves any counting, then it cannot currently
be done in pet or ppcg since isl does not support counting yet
and the public version of barvinok is GPL licensed.

Also, it's better to ask such questions on the isl mailing list
isl-developm...@googlegroups.com

skimo


isl scheduler and spatial locality (Re: [PATCH][GRAPHITE] More TLC)

2017-09-29 Thread Sven Verdoolaege
On Wed, Sep 27, 2017 at 02:18:51PM +0200, Richard Biener wrote:
> Ah, so I now see why we do not perform interchange on trivial cases like
> 
> double A[1024][1024], B[1024][1024];
> 
> void foo(void)
> {
>   for (int i = 0; i < 1024; ++i)
> for (int j = 0; j < 1024; ++j)
>   A[j][i] = B[j][i];
> }

I didn't see you mentioning _why_ you expect an interchange here.
Are you prehaps interested in spatial locality?
If so, then there are several approaches for taking
that into account.
- pluto performs an intra-tile loop interchange to
  improve temporal and/or spatial locality.  It shouldn't
  be too hard to do something similar on an isl generated
  schedule
- Alex (Oleksandr) has been working on an extension of
  the isl scheduler that takes into account spatial locality.
  I'm not sure if it's publicly available.
- I've been working on a special case of spatial locality
  (consecutivity).  The current version is available in
  the consecutivity branch.  Note that it may get rebased and
  it may not necessarily get merged into master.

There are also other approaches, but they may not be that
easy to combine with the isl scheduler.

skimo


isl scheduler and spatial locality (Re: [PATCH][GRAPHITE] More TLC)

2017-09-29 Thread Sven Verdoolaege
[Sorry for the resend; I used the wrong email address to CC Alex]

On Wed, Sep 27, 2017 at 02:18:51PM +0200, Richard Biener wrote:
> Ah, so I now see why we do not perform interchange on trivial cases like
> 
> double A[1024][1024], B[1024][1024];
> 
> void foo(void)
> {
>   for (int i = 0; i < 1024; ++i)
> for (int j = 0; j < 1024; ++j)
>   A[j][i] = B[j][i];
> }

I didn't see you mentioning _why_ you expect an interchange here.
Are you prehaps interested in spatial locality?
If so, then there are several approaches for taking
that into account.
- pluto performs an intra-tile loop interchange to
  improve temporal and/or spatial locality.  It shouldn't
  be too hard to do something similar on an isl generated
  schedule
- Alex (Oleksandr) has been working on an extension of
  the isl scheduler that takes into account spatial locality.
  I'm not sure if it's publicly available.
- I've been working on a special case of spatial locality
  (consecutivity).  The current version is available in
  the consecutivity branch.  Note that it may get rebased and
  it may not necessarily get merged into master.

There are also other approaches, but they may not be that
easy to combine with the isl scheduler.

skimo


Re: isl scheduler and spatial locality (Re: [PATCH][GRAPHITE] More TLC)

2017-10-01 Thread Sven Verdoolaege
On Sat, Sep 30, 2017 at 07:47:43PM +0200, Richard Biener wrote:
> On September 29, 2017 9:58:41 PM GMT+02:00, Sebastian Pop  
> wrote:
> >On Fri, Sep 29, 2017 at 2:37 PM, Sven Verdoolaege
> > wrote:
> >> [Sorry for the resend; I used the wrong email address to CC Alex]
> >>
> >> On Wed, Sep 27, 2017 at 02:18:51PM +0200, Richard Biener wrote:
> >>> Ah, so I now see why we do not perform interchange on trivial cases
> >like
> >>>
> >>> double A[1024][1024], B[1024][1024];
> >>>
> >>> void foo(void)
> >>> {
> >>>   for (int i = 0; i < 1024; ++i)
> >>> for (int j = 0; j < 1024; ++j)
> >>>   A[j][i] = B[j][i];
> >>> }
> >>
> >> I didn't see you mentioning _why_ you expect an interchange here.
> >> Are you prehaps interested in spatial locality?
> >> If so, then there are several approaches for taking
> >> that into account.
> >> - pluto performs an intra-tile loop interchange to
> >>   improve temporal and/or spatial locality.  It shouldn't
> >>   be too hard to do something similar on an isl generated
> >>   schedule
> >> - Alex (Oleksandr) has been working on an extension of
> >>   the isl scheduler that takes into account spatial locality.
> >>   I'm not sure if it's publicly available.
> >> - I've been working on a special case of spatial locality
> >>   (consecutivity).  The current version is available in
> >>   the consecutivity branch.  Note that it may get rebased and
> >>   it may not necessarily get merged into master.
> >>
> >> There are also other approaches, but they may not be that
> >> easy to combine with the isl scheduler.
> >
> >Would the following work?
> >Add to the proximity relation the array accesses from two
> >successive iterations of the innermost loop:
> >A[j][i] -> A[j][i+1] and B[j][i] -> B[j][i+1]
> >With these two extra relations in the proximity map,
> >isl should be able to interchange the above loop.
> 
> Can anyone give a hint on how to do that in ISL terms? 

For the approach pluto is taking, you'll have to look at the source
code, see pluto_intra_tile_optimize_band.
For the other two approaches I mentioned above, reports will
be made available within the next couple of weeks.
For the last one, there is a sample implementation in the
consecutivity branch of PPCG.

skimo


Re: [graphite] Move to cloog.org interface

2011-08-11 Thread Sven Verdoolaege
On Thu, Aug 11, 2011 at 01:16:45PM -0500, Sebastian Pop wrote:
> +  if (1)
> +{
> +  /* For now remove the isl_id's from the context before
> +  translating to CLooG: this code will be removed when the
> +  domain will also contain isl_id's.  */
> +  isl_set *context = isl_set_project_out (isl_set_copy (scop->context),
> +   isl_dim_set, 0, number_of_loops 
> ());
> +  isl_printer *p = isl_printer_to_str (scop->ctx);
> +  char *str;
> +
> +  p = isl_printer_set_output_format (p, ISL_FORMAT_EXT_POLYLIB);
> +  p = isl_printer_print_set (p, context);
> +  isl_set_free (context);
> +
> +  str = isl_printer_get_str (p);
> +  context = isl_set_read_from_str (scop->ctx, str,
> +scop_nb_params (scop));
> +  free (str);
> +  isl_printer_free (p);

Hmm are you saying you would like a isl_set_reset_dim_id?

> @@ -415,4 +416,40 @@ openscop_read_polyhedron_matrix (FILE *file, 
> ppl_Polyhedron_t *ph,
>  }
>  }
>  
> +/* Prints an isl_set S to stderr.  */
> +
> +DEBUG_FUNCTION void
> +debug_isl_set (isl_set *s)

You could use isl_set_dump.
It's not documented because I don't want people to depend on the output,
but for debugging it should be just fine.

> @@ -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);
> +

isl_set_free will handle NULL input just fine.

> +static isl_pw_aff *
> +extract_affine_chrec (scop_p s, tree e)
> +{
> +  isl_pw_aff *lhs = extract_affine (s, CHREC_LEFT (e));
> +  isl_pw_aff *rhs = extract_affine (s, CHREC_RIGHT (e));
> +  isl_dim *dim = isl_dim_set_alloc (s->ctx, 0, number_of_loops ());
> +  isl_local_space *ls = isl_local_space_from_dim (dim);
> +  isl_aff *loop = isl_aff_set_coefficient_si
> +(isl_aff_zero (ls), isl_dim_set, CHREC_VARIABLE (e), 1);
> +
> +  return isl_pw_aff_add (lhs,
> +  isl_pw_aff_mul (rhs, isl_pw_aff_from_aff (loop)));

You should test that at least one of your arguments is constant.
Alternatively, if you want to construct polynomials, you should
use isl_pw_qpolynomials instead.

> +  isl_set *inner = isl_set_copy (outer);
> +  isl_dim *d = isl_set_get_dim (scop->context);
> +  isl_id *id = isl_id_for_loop (scop, loop);
> +  int pos = isl_dim_find_dim_by_id (d, isl_dim_set, id);

This is dangerous.  You cannot depend on non-parameters
keeping their ids across operations.  Don't you already
know the position somehow?  When you are using PPL,
you must keep track of this information, no?

> +
> +  /* FIXME: This function will be renamed isl_map_insert_dims and
> + documented in a later version of ISL (current ISL is 0.07).  */

Since you are using isl_ids, 0.07 won't work for you.

> +   /* Make the dimension of LB and UB to be exactly NBS.  */
> +   lb = isl_pw_aff_drop_dims (lb, isl_dim_set, 0, nbl - 1);
> +   ub = isl_pw_aff_drop_dims (ub, isl_dim_set, 0, nbl - 1);
> +   lb = isl_pw_aff_add_dims (lb, isl_dim_set, nbs - 1);
> +   ub = isl_pw_aff_add_dims (ub, isl_dim_set, nbs - 1);

This looks fishy.  Are you sure the expressions don't depend on the
set variables?

> +  {
> +isl_dim *dc = isl_set_get_dim (scop->context);
> +int nb_in = isl_dim_size (dc, isl_dim_set);
> +int nb_out = 1 + DR_NUM_DIMENSIONS (dr);
> +int nbp = scop_nb_params (scop);
> +isl_dim *dim = isl_dim_alloc (scop->ctx, nbp, nb_in, nb_out);
> +int i;
> +
> +for (i = 0; i < nbp; i++)
> +  dim = isl_dim_set_dim_id (dim, isl_dim_param, i,
> + isl_dim_get_dim_id (dc, isl_dim_param, i));
> +for (i = 0; i < nb_in; i++)
> +  dim = isl_dim_set_dim_id (dim, isl_dim_in, i,
> + isl_dim_get_dim_id (dc, isl_dim_set, i));

This is dangerous too.  Why don't you derive dim directly from dc
instead of creating a fresh dim and then trying to copy some information?

skimo


Re: [graphite] Move to cloog.org interface

2011-08-11 Thread Sven Verdoolaege
On Thu, Aug 11, 2011 at 08:14:05PM +0100, Tobias Grosser wrote:
> I think the best would be to provide our ctx to cloog when allocating
> the CloogState.

Yes.

skimo


Re: [graphite] Move to cloog.org interface

2011-08-11 Thread Sven Verdoolaege
On Thu, Aug 11, 2011 at 03:10:30PM -0500, Sebastian Pop wrote:
> On Thu, Aug 11, 2011 at 14:28, Sven Verdoolaege  wrote:
> > On Thu, Aug 11, 2011 at 01:16:45PM -0500, Sebastian Pop wrote:
> >> +
> >> +  /* FIXME: This function will be renamed isl_map_insert_dims and
> >> +     documented in a later version of ISL (current ISL is 0.07).  */
> >
> > Since you are using isl_ids, 0.07 won't work for you.
> 
> For now I'm using the ISL that is distributed with cloog-isl.

Which version?

> What version of ISL should I use to have isl_ids working?

0.08.  While you are developing, you can use the latest
git version and just tell CLooG to use that version.
While I will not be maintaining CLooG this year, I will
provide updates if I change isl in some incompatible way.

skimo


Re: [graphite] Move to cloog.org interface

2011-08-11 Thread Sven Verdoolaege
On Thu, Aug 11, 2011 at 03:23:13PM -0500, Sebastian Pop wrote:
> As we are using this function only on parameters, get_name should
> return a unique name.  I guess that the name in isl_id is only used
> for debugging purposes, as the ISL manual states that "Identifiers
> with the same name but different pointer values are considered to
> be distinct."

Identifiers with different names are (obviously) also considered
to be distinct.

skimo


Re: [graphite] Move to cloog.org interface

2011-08-11 Thread Sven Verdoolaege
On Thu, Aug 11, 2011 at 04:59:43PM -0500, Sebastian Pop wrote:
> >>> +  {
> >>> +    isl_dim *dc = isl_set_get_dim (scop->context);
> >>> +    int nb_in = isl_dim_size (dc, isl_dim_set);
> >>> +    int nb_out = 1 + DR_NUM_DIMENSIONS (dr);
> >>> +    int nbp = scop_nb_params (scop);
> >>> +    isl_dim *dim = isl_dim_alloc (scop->ctx, nbp, nb_in, nb_out);
> >>> +    int i;
> >>> +
> >>> +    for (i = 0; i < nbp; i++)
> >>> +      dim = isl_dim_set_dim_id (dim, isl_dim_param, i,
> >>> +                             isl_dim_get_dim_id (dc, isl_dim_param, i));
> >>> +    for (i = 0; i < nb_in; i++)
> >>> +      dim = isl_dim_set_dim_id (dim, isl_dim_in, i,
> >>> +                             isl_dim_get_dim_id (dc, isl_dim_set, i));
> >>
> >> This is dangerous too.  Why don't you derive dim directly from dc
> >> instead of creating a fresh dim and then trying to copy some information?
> >
> > Yes, thanks for pointing this out.  I will fix this.
> 
> I am confused on this one:
> which function should I use to create dim from dc?

It looks like you want to do

dim = isl_dim_add(isl_dim_from_domain(dc), isl_dim_out, nb_out);

skimo


Re: [PATCH 08/11] Add ISL data structures

2011-08-12 Thread Sven Verdoolaege
Shouldn't you document that you need isl now?

skimo


Re: [PATCH 09/11] Add scop->context

2011-08-12 Thread Sven Verdoolaege
On Thu, Aug 11, 2011 at 05:44:37PM -0500, Sebastian Pop wrote:
> +  ctx = isl_ctx_alloc ();

I can't find the call to cloog_isl_state_malloc in this patch
that Tobi correctly requested.

skimo


Re: [PATCH 2/2] document ISL requirement for GCC installation

2011-08-12 Thread Sven Verdoolaege
On Fri, Aug 12, 2011 at 10:16:05AM -0500, Sebastian Pop wrote:
> ---
>  gcc/doc/install.texi |8 +++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
> index 368221f..f2b2fd9 100644
> --- a/gcc/doc/install.texi
> +++ b/gcc/doc/install.texi
> @@ -368,6 +368,11 @@ It can be downloaded from 
> @uref{http://www.cs.unipr.it/ppl/Download/}.
>  The configure option @option{--with-ppl} should be used if PPL is not
>  installed in your default library search path.
>  
> +@item Integer Set Library (ISL) version 0.08

Are you going to wait until 0.08 is available before
applying these patches?

> +Necessary to build GCC with the Graphite loop optimizations.
> +It can be downloaded from @uref{http://www.kotnet.org/~skimo/isl/}.

Please use http://freshmeat.net/projects/isl/ instead.

skimo


Re: [PATCH 2/2] document ISL requirement for GCC installation

2011-08-12 Thread Sven Verdoolaege
On Fri, Aug 12, 2011 at 05:02:18PM +, Joseph S. Myers wrote:
> On Fri, 12 Aug 2011, Sebastian Pop wrote:
> > +@item Integer Set Library (ISL) version 0.08
> > +
> > +Necessary to build GCC with the Graphite loop optimizations.
> > +It can be downloaded from @uref{http://www.kotnet.org/~skimo/isl/}.
> 
> So have things changed relative to what was said in 
>  about ISL being 
> included in CLooG-ISL?

isl is still included in CLooG, but Sebastian now wants to use isl
itself in graphite.

skimo


Re: [PATCH 2/2] document ISL requirement for GCC installation

2011-08-12 Thread Sven Verdoolaege
On Fri, Aug 12, 2011 at 06:56:38PM +, Joseph S. Myers wrote:
> I don't see why that should make any difference to the build requirements.  
> If CLooG-ISL builds and installs a library libisl.a as well as 
> libcloog-isl.a (as config/cloog.m4 thinks it does at present), why should 
> someone need to download and install a separate ISL package rather than 
> getting libisl.a from CLooG-ISL?

The one that comes with CLooG may not be recent enough.

skimo


Re: [PATCH 2/2] document ISL requirement for GCC installation

2011-08-12 Thread Sven Verdoolaege
On Fri, Aug 12, 2011 at 07:16:55PM +, Joseph S. Myers wrote:
> Do you mean there is not only a requirement to build both libraries, but 
> there is a requirement to build CLooG *first*, then ISL, so that ISL's 
> libisl.a overwrites CLooG's rather than the other way round (supposing 
> that they are installed in the same prefix)?

No.  You build isl first and configure CLooG to use that isl.

skimo


Re: [PATCH 2/2] document ISL requirement for GCC installation

2011-08-12 Thread Sven Verdoolaege
On Fri, Aug 12, 2011 at 07:28:52PM +, Joseph S. Myers wrote:
> On Fri, 12 Aug 2011, Sven Verdoolaege wrote:
> 
> > On Fri, Aug 12, 2011 at 07:16:55PM +, Joseph S. Myers wrote:
> > > Do you mean there is not only a requirement to build both libraries, but 
> > > there is a requirement to build CLooG *first*, then ISL, so that ISL's 
> > > libisl.a overwrites CLooG's rather than the other way round (supposing 
> > > that they are installed in the same prefix)?
> > 
> > No.  You build isl first and configure CLooG to use that isl.
> 
> Will CLooG give an error if you configure it in such a way that its isl 
> would overwrite one previously installed?  If not, this seems too 
> error-prone - there's no obvious way for CLooG to know whether a 
> previously installed isl comes from a previous installation of CLooG 
> (should be overwritten) or was installed on its own (so CLooG should be 
> built to use it).

If CLooG is told to use a previously built isl, it won't even compile
the bundled isl.

skimo


Re: [PATCH 2/2] document ISL requirement for GCC installation

2011-08-12 Thread Sven Verdoolaege
On Fri, Aug 12, 2011 at 03:30:25PM -0400, Jack Howarth wrote:
> Skimo,
>Currently we don't have any checks for the minimal isl version required.

I assume they will be added at some point.
AFAIU, Sebastian just started working on this.
It will take some time for him to finish the transition.

Anyway, I'm not involved in graphite development.
I was just asked to review some patches on their use of isl.

skimo


Re: [PATCH 15/20] strip-mine with isl

2011-08-15 Thread Sven Verdoolaege
On Mon, Aug 15, 2011 at 02:12:54AM -0500, Sebastian Pop wrote:
> @@ -160,6 +182,14 @@ pbb_strip_mine_time_depth (poly_bb_p pbb, int 
> time_depth, int stride)
>  ppl_delete_Linear_Expression (expr);
>  ppl_Polyhedron_add_constraint (res, new_cstr);
>  ppl_delete_Constraint (new_cstr);
> +
> +{
> +  isl_dim *d = isl_map_get_dim (pbb->transformed);
> +  isl_constraint *c = isl_equality_alloc (d);
> +
> +  c = isl_constraint_set_coefficient_si (c, isl_dim_out, strip + 1, 1);
> +  pbb->transformed = isl_map_add_constraint (pbb->transformed, c);
> +}
>}
>  }

I'm not sure what the point is of introducing an extra dimension
fixed to zero, but it can be accomplished more easily using isl_map_fix_si.
Also, it may clarify the code if you first construct the transformation
and then apply it, like you do in the interchange.

skimo