This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: {GRAPHITE] Replacement of isl_int by isl_val


On Sun, Feb 9, 2014 at 9:19 PM, Tobias Grosser <tobias@grosser.es> wrote:
> On 02/09/2014 04:47 PM, Mircea Namolaru wrote:
>>
>>    Patch for replacement of the isl_int (obsolete) by isl_val.
>>
>>    No regressions for c/c++/fortran on x86-64 Linux.
>
>
> Hi Mircae,
>
> thanks a lot for looking into this.
>
> Just for people not aware of the idea of this patch. Cloog recently upgraded
> to the latest version of isl (isl-0.12.2) and with this
> release isl deprecated the isl_int interface in favor of the isl_val
> interface.

It seems that neither ISL 0.10 nor 0.11 or 0.11.1 has isl_val though, so
this patch would need to adjust the ISL version check in the toplevel
configure.

Which also makes this patch not suitable for this stage but it has to
wait for stage1.

Btw, Mircea - do you have a copyright assignment on file with the FSF
covering work on GCC?

Thanks,
Richard.

> Both are interfaces for arbitrary precision numbers. However,
> the old isl_int interface was mostly a proxy to gmp, whereas the new
> interface hides the implementation and will later allow optimizations for
> the common use case of small integers.
>
> We really want to get to the latest version of isl as the newer releases
> (and development branches) provide very useful features
> including the possibility to bound the amount of computation before
> we fall back to a more conservative solution. This feature should help us to
> fix some of the newer bugs.
>
> I have doubts if we can still push this patch to graphite at this stage of
> trunk, but at the very least we should have it ready for stage-1.
>
> Regarding your patch. It looks generally very nice. Just a couple of smaller
> comments:
>
> - Can you adapt configure to only allow isl/cloog versions that actually
> support the new interface?
>
> - Can you updated the documentation that lists the supported isl/cloog
>   versions.
>
> Besides some inline comments:
>
>
>> Index: gcc/graphite-optimize-isl.c
>> ===================================================================
>> --- gcc/graphite-optimize-isl.c (revision 207298)
>> +++ gcc/graphite-optimize-isl.c (working copy)
>> @@ -260,6 +260,7 @@
>>      DimToVectorize can be devided by VectorWidth. The default VectorWidth
>> is
>>      currently constant and not yet target specific. This function does
>> not reason
>>      about parallelism.  */
>> +
>
>
> This change looks unrelated. Can you submit a separate fix?
>
>>   static isl_map *
>>   getPrevectorMap (isl_ctx *ctx, int DimToVectorize,
>>                  int ScheduleDimensions,
>> @@ -273,8 +274,9 @@
>>     isl_aff *Aff;
>>     int PointDimension; /* ip */
>>     int TileDimension;  /* it */
>> -  isl_int VectorWidthMP;
>> +  isl_val *VectorWidthMP;
>>     int i;
>> +  isl_ctx *ct;
>
>
> We normally use 'ctx' as abbreviation for context.
>
>>
>>     /* assert (0 <= DimToVectorize && DimToVectorize <
>> ScheduleDimensions);*/
>>
>> @@ -304,10 +306,10 @@
>>     Aff = isl_aff_zero_on_domain (LocalSpaceRange);
>>     Aff = isl_aff_set_constant_si (Aff, VectorWidth);
>>     Aff = isl_aff_set_coefficient_si (Aff, isl_dim_in, TileDimension, 1);
>> -  isl_int_init (VectorWidthMP);
>> -  isl_int_set_si (VectorWidthMP, VectorWidth);
>> -  Aff = isl_aff_mod (Aff, VectorWidthMP);
>> -  isl_int_clear (VectorWidthMP);
>> +
>> +  ct = isl_aff_get_ctx (Aff);
>> +  VectorWidthMP = isl_val_int_from_si (ct, VectorWidth);
>> +  Aff = isl_aff_mod_val (Aff, VectorWidthMP);
>>     Modulo = isl_pw_aff_zero_set (isl_pw_aff_from_aff (Aff));
>>     TilingMap = isl_map_intersect_range (TilingMap, Modulo);
>>
>> Index: gcc/graphite-sese-to-poly.c
>> ===================================================================
>> --- gcc/graphite-sese-to-poly.c (revision 207298)
>> +++ gcc/graphite-sese-to-poly.c (working copy)
>> @@ -26,8 +26,15 @@
>>   #include <isl/union_map.h>
>>   #include <isl/constraint.h>
>>   #include <isl/aff.h>
>> +#include <isl/val.h>
>> +#if defined(__cplusplus)
>> +extern "C" {
>> +#endif
>> +#include <isl/val_gmp.h>
>> +#if defined(__cplusplus)
>> +}
>
>
> We should put a comment why this is extern "C" is necessary. Or better, we
> should check if Sven can get release us an isl 0.12.3 that includes the
> extern "C" in the relevant header"
>
>> +#endif
>>   #include <cloog/cloog.h>
>> -#include <cloog/cloog.h>
>
>
> This seems to be an unrelated fix. Can you submit a separate patch?
>
>>   #include <cloog/isl/domain.h>
>>   #endif
>>
>> @@ -67,7 +74,6 @@
>>   #include "graphite-poly.h"
>>   #include "graphite-sese-to-poly.h"
>>
>> -
>
>
> This change looks unrelated. Can you submit a separate fix?
>
>>   /* Assigns to RES the value of the INTEGER_CST T.  */
>>
>>   static inline void
>> @@ -481,13 +487,11 @@
>>     int i;
>>     int nb_iterators = pbb_dim_iter_domain (pbb);
>>     int used_scattering_dimensions = nb_iterators * 2 + 1;
>> -  isl_int val;
>> +  isl_val *val;
>>     isl_space *dc, *dm;
>>
>>     gcc_assert (scattering_dimensions >= used_scattering_dimensions);
>>
>> -  isl_int_init (val);
>> -
>>     dc = isl_set_get_space (pbb->domain);
>>     dm = isl_space_add_dims (isl_space_from_domain (dc),
>>                            isl_dim_out, scattering_dimensions);
>> @@ -501,12 +505,10 @@
>>           isl_constraint *c = isl_equality_alloc
>>               (isl_local_space_from_space (isl_map_get_space
>> (pbb->schedule)));
>>
>> -         if (0 != isl_aff_get_coefficient (static_sched, isl_dim_in,
>> -                                           i / 2, &val))
>> -           gcc_unreachable ();
>> +         val = isl_aff_get_coefficient_val (static_sched, isl_dim_in, i /
>> 2);
>>
>> -         isl_int_neg (val, val);
>> -         c = isl_constraint_set_constant (c, val);
>> +         val = isl_val_neg (val);
>> +         c = isl_constraint_set_constant_val (c, val);
>>           c = isl_constraint_set_coefficient_si (c, isl_dim_out, i, 1);
>>           pbb->schedule = isl_map_add_constraint (pbb->schedule, c);
>>         }
>> @@ -520,8 +522,6 @@
>>         }
>>       }
>>
>> -  isl_int_clear (val);
>> -
>>     pbb->transformed = isl_map_copy (pbb->schedule);
>>   }
>>
>> @@ -700,12 +700,12 @@
>>     isl_local_space *ls = isl_local_space_from_space (isl_space_copy
>> (space));
>>     isl_aff *aff = isl_aff_zero_on_domain (ls);
>>     isl_set *dom = isl_set_universe (space);
>> -  isl_int v;
>> +  isl_val *v;
>> +  isl_ctx *ct;
>>
>> -  isl_int_init (v);
>> -  isl_int_set_gmp (v, g);
>> -  aff = isl_aff_add_constant (aff, v);
>> -  isl_int_clear (v);
>> +  ct = isl_aff_get_ctx (aff);
>> +  v = isl_val_int_from_gmp (ct, g);
>> +  aff = isl_aff_add_constant_val (aff, v);
>>
>>     return isl_pw_aff_alloc (dom, aff);
>>   }
>> @@ -728,19 +728,17 @@
>>
>>   /* Compute pwaff mod 2^width.  */
>>
>> +extern isl_ctx *the_isl_ctx;
>
>
> Why is this one needed?
>
>>   static isl_pw_aff *
>>   wrap (isl_pw_aff *pwaff, unsigned width)
>>   {
>> -  isl_int mod;
>> +  isl_val *mod;
>>
>> -  isl_int_init (mod);
>> -  isl_int_set_si (mod, 1);
>> -  isl_int_mul_2exp (mod, mod, width);
>> +  mod = isl_val_int_from_ui(the_isl_ctx, width);
>> +  mod = isl_val_2exp (mod);
>> +  pwaff = isl_pw_aff_mod_val (pwaff, mod);
>>
>> -  pwaff = isl_pw_aff_mod (pwaff, mod);
>> -
>> -  isl_int_clear (mod);
>> -
>>     return pwaff;
>>   }
>>
>> @@ -995,11 +993,10 @@
>>     isl_space *space;
>>     isl_constraint *c;
>>     int pos = isl_set_dim (outer, isl_dim_set);
>> -  isl_int v;
>> +  isl_val *v;
>>     mpz_t g;
>>
>>     mpz_init (g);
>> -  isl_int_init (v);
>>
>>     inner = isl_set_add_dims (inner, isl_dim_set, 1);
>>     space = isl_set_get_space (inner);
>> @@ -1017,8 +1014,8 @@
>>           (isl_local_space_from_space (isl_space_copy (space)));
>>         c = isl_constraint_set_coefficient_si (c, isl_dim_set, pos, -1);
>>         tree_int_to_gmp (nb_iters, g);
>> -      isl_int_set_gmp (v, g);
>> -      c = isl_constraint_set_constant (c, v);
>> +      v = isl_val_int_from_gmp (the_isl_ctx, g);
>> +      c = isl_constraint_set_constant_val (c, v);
>>         inner = isl_set_add_constraint (inner, c);
>>       }
>>
>> @@ -1072,9 +1069,9 @@
>>           c = isl_inequality_alloc
>>               (isl_local_space_from_space (isl_space_copy (space)));
>>           c = isl_constraint_set_coefficient_si (c, isl_dim_set, pos, -1);
>> -         isl_int_set_gmp (v, g);
>> +         v = isl_val_int_from_gmp (the_isl_ctx, g);
>>           mpz_clear (g);
>> -         c = isl_constraint_set_constant (c, v);
>> +         c = isl_constraint_set_constant_val (c, v);
>>           inner = isl_set_add_constraint (inner, c);
>>         }
>>         else
>> @@ -1097,7 +1094,6 @@
>>
>>     isl_set_free (outer);
>>     isl_space_free (space);
>> -  isl_int_clear (v);
>>     mpz_clear (g);
>>   }
>>
>> @@ -1331,17 +1327,15 @@
>>         isl_space *space = isl_set_get_space (scop->context);
>>         isl_constraint *c;
>>         mpz_t g;
>> -      isl_int v;
>> +      isl_val *v;
>>
>>         c = isl_inequality_alloc (isl_local_space_from_space (space));
>>         mpz_init (g);
>> -      isl_int_init (v);
>>         tree_int_to_gmp (lb, g);
>> -      isl_int_set_gmp (v, g);
>> -      isl_int_neg (v, v);
>> +      v = isl_val_int_from_gmp (the_isl_ctx, g);
>> +      v = isl_val_neg (v);
>>         mpz_clear (g);
>> -      c = isl_constraint_set_constant (c, v);
>> -      isl_int_clear (v);
>> +      c = isl_constraint_set_constant_val (c, v);
>>         c = isl_constraint_set_coefficient_si (c, isl_dim_param, p, 1);
>>
>>         scop->context = isl_set_add_constraint (scop->context, c);
>> @@ -1352,17 +1346,15 @@
>>         isl_space *space = isl_set_get_space (scop->context);
>>         isl_constraint *c;
>>         mpz_t g;
>> -      isl_int v;
>> +      isl_val *v;
>>
>>         c = isl_inequality_alloc (isl_local_space_from_space (space));
>>
>>         mpz_init (g);
>> -      isl_int_init (v);
>>         tree_int_to_gmp (ub, g);
>> -      isl_int_set_gmp (v, g);
>> +      v = isl_val_int_from_gmp (the_isl_ctx, g);
>>         mpz_clear (g);
>> -      c = isl_constraint_set_constant (c, v);
>> -      isl_int_clear (v);
>> +      c = isl_constraint_set_constant_val (c, v);
>>         c = isl_constraint_set_coefficient_si (c, isl_dim_param, p, -1);
>>
>>         scop->context = isl_set_add_constraint (scop->context, c);
>> Index: gcc/graphite-poly.c
>> ===================================================================
>> --- gcc/graphite-poly.c (revision 207298)
>> +++ gcc/graphite-poly.c (working copy)
>> @@ -28,6 +28,14 @@
>>   #include <isl/constraint.h>
>>   #include <isl/ilp.h>
>>   #include <isl/aff.h>
>> +#include <isl/val.h>
>> +#if defined(__cplusplus)
>> +extern "C" {
>> +#endif
>> +#include <isl/val_gmp.h>
>> +#if defined(__cplusplus)
>> +}
>> +#endif
>
>
> Let's check with Sven for a newer version. Or lets keep it in case we can
> verify this one does not break if the same pattern is added to isl at some
> point and we get a nested 'extern "C"'.
>
>>   #include <cloog/cloog.h>
>>   #include <cloog/isl/domain.h>
>>   #endif
>> @@ -1029,11 +1037,8 @@
>>     isl_set *transdomain;
>>     isl_space *dc;
>>     isl_aff *aff;
>> -  isl_int isllb, islub;
>> +  isl_val *isllb, *islub;
>>
>> -  isl_int_init (isllb);
>> -  isl_int_init (islub);
>> -
>>     /* Map the iteration domain through the current scatter, and work
>>        on the resulting set.  */
>>     transdomain = isl_set_apply (isl_set_copy (pbb->domain),
>> @@ -1046,15 +1051,14 @@
>>
>>     /* And find the min/max for that function.  */
>>     /* XXX isl check results?  */
>> -  isl_set_min (transdomain, aff, &isllb);
>> -  isl_set_max (transdomain, aff, &islub);
>> +  isllb = isl_set_min_val (transdomain, aff);
>> +  islub = isl_set_max_val (transdomain, aff);
>>
>> -  isl_int_sub (islub, islub, isllb);
>> -  isl_int_add_ui (islub, islub, 1);
>> -  isl_int_get_gmp (islub, res);
>> +  islub = isl_val_sub (islub, isllb);
>> +  islub = isl_val_add_ui (islub, 1);
>> +  isl_val_get_num_gmp (islub, res);
>>
>> -  isl_int_clear (isllb);
>> -  isl_int_clear (islub);
>> +  isl_val_free (islub);
>>     isl_aff_free (aff);
>>     isl_set_free (transdomain);
>>   }
>> Index: gcc/graphite-interchange.c
>> ===================================================================
>> --- gcc/graphite-interchange.c  (revision 207635)
>> +++ gcc/graphite-interchange.c  (working copy)
>> @@ -29,6 +29,14 @@
>>   #include <isl/map.h>
>>   #include <isl/union_map.h>
>>   #include <isl/ilp.h>
>> +#include <isl/val.h>
>> +#if defined(__cplusplus)
>> +extern "C" {
>> +#endif
>> +#include <isl/val_gmp.h>
>> +#if defined(__cplusplus)
>> +}
>> +#endif
>>   #include <cloog/cloog.h>
>>   #include <cloog/isl/domain.h>
>>   #endif
>> @@ -79,13 +87,13 @@
>>     isl_local_space *ls = isl_local_space_from_space (isl_map_get_space
>> (map));
>>     unsigned offset, nsubs;
>>     int i;
>> -  isl_int size, subsize;
>> +  isl_ctx *ct;
>
>
> Use 'ctx'
>
>>
>> +  isl_val *size, *subsize, *size1;
>> +
>>     res = isl_equality_alloc (ls);
>> -  isl_int_init (size);
>> -  isl_int_set_ui (size, 1);
>> -  isl_int_init (subsize);
>> -  isl_int_set_ui (subsize, 1);
>> +  ct = isl_local_space_get_ctx (ls);
>> +  size = isl_val_int_from_ui (ct, 1);
>>
>>     nsubs = isl_set_dim (pdr->extent, isl_dim_set);
>>     /* -1 for the already included L dimension.  */
>> @@ -98,18 +106,17 @@
>>         isl_space *dc;
>>         isl_aff *aff;
>>
>> -      res = isl_constraint_set_coefficient (res, isl_dim_out, offset + i,
>> size);
>> -
>> +      size1 = isl_val_copy (size);
>> +      res = isl_constraint_set_coefficient_val (res, isl_dim_out, offset
>> + i, size);
>>         dc = isl_set_get_space (pdr->extent);
>>         aff = isl_aff_zero_on_domain (isl_local_space_from_space (dc));
>>         aff = isl_aff_set_coefficient_si (aff, isl_dim_in, i, 1);
>> -      isl_set_max (pdr->extent, aff, &subsize);
>> +      subsize = isl_set_max_val (pdr->extent, aff);
>>         isl_aff_free (aff);
>> -      isl_int_mul (size, size, subsize);
>> +      size = isl_val_mul (size1, subsize);
>>       }
>>
>> -  isl_int_clear (subsize);
>> -  isl_int_clear (size);
>> +  isl_val_free (size);
>>
>>     return res;
>>   }
>> @@ -126,7 +133,7 @@
>>     isl_aff *aff;
>>     isl_space *dc;
>>     isl_constraint *lma, *c;
>> -  isl_int islstride;
>> +  isl_val *islstride;
>>     graphite_dim_t time_depth;
>>     unsigned offset, nt;
>>     unsigned i;
>> @@ -239,10 +246,9 @@
>>     aff = isl_aff_zero_on_domain (isl_local_space_from_space (dc));
>>     aff = isl_aff_set_coefficient_si (aff, isl_dim_in, offset - 1, -1);
>>     aff = isl_aff_set_coefficient_si (aff, isl_dim_in, offset + offset -
>> 1, 1);
>> -  isl_int_init (islstride);
>> -  isl_set_max (set, aff, &islstride);
>> -  isl_int_get_gmp (islstride, stride);
>> -  isl_int_clear (islstride);
>> +  islstride = isl_set_max_val (set, aff);
>> +  isl_val_get_num_gmp (islstride, stride);
>> +  isl_val_free (islstride);
>>     isl_aff_free (aff);
>>     isl_set_free (set);
>>
>> Index: gcc/graphite-clast-to-gimple.c
>> ===================================================================
>> --- gcc/graphite-clast-to-gimple.c      (revision 207298)
>> +++ gcc/graphite-clast-to-gimple.c      (working copy)
>> @@ -28,6 +28,14 @@
>>   #include <isl/constraint.h>
>>   #include <isl/ilp.h>
>>   #include <isl/aff.h>
>> +#include <isl/val.h>
>> +#if defined(__cplusplus)
>> +extern "C" {
>> +#endif
>> +#include <isl/val_gmp.h>
>> +#if defined(__cplusplus)
>> +}
>> +#endif
>>   #include <cloog/cloog.h>
>>   #include <cloog/isl/domain.h>
>>   #endif
>> @@ -871,18 +879,18 @@
>>   static void
>>   compute_bounds_for_param (scop_p scop, int param, mpz_t low, mpz_t up)
>>   {
>> -  isl_int v;
>> +  isl_val *v;
>>     isl_aff *aff = isl_aff_zero_on_domain
>>       (isl_local_space_from_space (isl_set_get_space (scop->context)));
>>
>>     aff = isl_aff_add_coefficient_si (aff, isl_dim_param, param, 1);
>>
>> -  isl_int_init (v);
>> -  isl_set_min (scop->context, aff, &v);
>> -  isl_int_get_gmp (v, low);
>> -  isl_set_max (scop->context, aff, &v);
>> -  isl_int_get_gmp (v, up);
>> -  isl_int_clear (v);
>> +  v = isl_set_min_val (scop->context, aff);
>> +  isl_val_get_num_gmp (v, low);
>> +  isl_val_free (v);
>> +  v = isl_set_max_val (scop->context, aff);
>> +  isl_val_get_num_gmp (v, up);
>> +  isl_val_free (v);
>>     isl_aff_free (aff);
>>   }
>>
>> @@ -901,8 +909,7 @@
>>     isl_set *domain;
>>     isl_aff *dimension;
>>     isl_local_space *local_space;
>> -  isl_int isl_value;
>> -  enum isl_lp_result lp_result;
>> +  isl_val *isl_value;
>>
>>     domain = isl_set_copy (isl_set_from_cloog_domain (loop->domain));
>>     local_space = isl_local_space_from_space (isl_set_get_space (domain));
>> @@ -911,17 +918,12 @@
>>                                           isl_set_dim (domain,
>> isl_dim_set) - 1,
>>                                           1);
>>
>> -  isl_int_init (isl_value);
>> -
>> -  lp_result = isl_set_min (domain, dimension, &isl_value);
>> -  assert (lp_result == isl_lp_ok);
>> -  isl_int_get_gmp (isl_value, low);
>> -
>> -  lp_result = isl_set_max (domain, dimension, &isl_value);
>> -  assert (lp_result == isl_lp_ok);
>> -  isl_int_get_gmp (isl_value, up);
>> -
>> -  isl_int_clear (isl_value);
>> +  isl_value = isl_set_min_val (domain, dimension);
>> +  isl_val_get_num_gmp (isl_value, low);
>> +  isl_val_free (isl_value);
>> +  isl_value = isl_set_max_val (domain, dimension);
>> +  isl_val_get_num_gmp (isl_value, up);
>> +  isl_val_free (isl_value);
>>     isl_set_free (domain);
>>     isl_aff_free (dimension);
>>   }
>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]