On Thu, Aug 11, 2011 at 14:28, Sven Verdoolaege <sk...@kotnet.org> wrote:
> 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?
No thanks: this is just a hack that will disappear when all the data structs
will be in ISL format. I had this a bug with ISL complained during cloog
codegen that some maps had ids and some other maps did not.
>> + 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.
Ok, good to know, I remember also the manual warning on this point.
I think that, in this case, it is safe, as at this point we are working on
code that has already been filtered out of other things than affine
expressions. I should then assert that at least one of the args
is constant.
>> + 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.
Ok. Could you please document this in the ISL user manual?
> Don't you already know the position somehow?
Yes, I could be using the index of the loop (loop->num) here,
as the scop->context contains dimensions for all the existing
loops in the program.
>
>> +
>> + /* 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.
What version of ISL should I use to have isl_ids working?
>
>> + /* 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?
Yes, the lb and ub in this particular case are integers only:
the weird condition that we have just before ensures that.
+ if (host_integerp (low, 0)
+ && high
+ && host_integerp (high, 0)
+ /* 1-element arrays at end of structures may extend over
+ their declared size. */
+ && !(array_at_struct_end_p (ref)
+ && operand_equal_p (low, high, 0)))
>
>> + {
>> + 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.
Thanks,
Sebastian
