| Message ID | CAFk3UF8GpyZ-Qb8X6iGVQAmZq_ZZmweY4OJE4-_t8GV--hmysA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
On Thu, Aug 11, 2011 at 14:28, Sven Verdoolaege <skimo@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
On Thu, Aug 11, 2011 at 14:14, Tobias Grosser <tobias@grosser.es> wrote: > This needs to be adapted to my cloog.org interface patch. I will adapt my patch set to be on top of your patch. >> +/* 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? 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." >> @@ -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 This change is in untested "feature" (read "bug") code: there are no testcase, and the code of openscop is not enabled in trunk. I will remove the code for openscop and let somebody else do the ISL port for it and re-enable if needed. Sebastian
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
On Thu, Aug 11, 2011 at 03:10:30PM -0500, Sebastian Pop wrote: > On Thu, Aug 11, 2011 at 14:28, Sven Verdoolaege <skimo@kotnet.org> 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
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
On Thu, Aug 11, 2011 at 16:00, Sven Verdoolaege <skimo@kotnet.org> wrote: > On Thu, Aug 11, 2011 at 03:10:30PM -0500, Sebastian Pop wrote: >> On Thu, Aug 11, 2011 at 14:28, Sven Verdoolaege <skimo@kotnet.org> 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? commit 225c2ed62fe37a4db22bf4b95c3731dab1a50dde Author: Sven Verdoolaege <skimo@kotnet.org> Date: Sun Jul 10 09:27:24 2011 +0200 CLooG 0.16.3 Signed-off-by: Sven Verdoolaege <skimo@kotnet.org> >> 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. Ok, I think I do use a quite recent version: commit 3fbc27ff19492e6ae0975ac26d006a7d93ebe39b Author: Sven Verdoolaege <skimo@kotnet.org> Date: Sun Jul 31 12:57:01 2011 +0200 isl_aff_floor: reduce coefficients of newly created div Signed-off-by: Sven Verdoolaege <skimo@kotnet.org> I will still pull the latest changes from ISL. > While I will not be maintaining CLooG this year, I will > provide updates if I change isl in some incompatible way. Ok. Thanks, Sebastian
>>> + { >>> + 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? I don't see how to specify the number of input and output dimensions like the isl_dim_alloc would do. What do you think about copying only the parameters from scop->context? { 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)); acc = isl_map_universe (dim); acc = isl_map_set_tuple_id (acc, isl_dim_out, isl_id_for_dr (scop, dr)); isl_dim_free (dc); } Would this be ok? Thanks, Sebastian
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
Hi, here are the updated patches for the conversion of Graphite to ISL following the comments from Tobias and Sven. The patches are passing regression testing on amd64-linux. cd .../build/gcc/ make -k check RUNTESTFLAGS=graphite.exp cd .../build/x86_64-unknown-linux-gnu/libgomp/testsuite make -k check RUNTESTFLAGS=graphite.exp Thanks, Sebastian Sebastian Pop (11): Make CLooG-ISL the only supported CLooG version. Require cloog 0.16.3 Remove code that supported legacy CLooG. Document CLooG-ISL requirement for Graphite Move to new Cloog interface. Remove ATTRIBUTE_UNUSED fix memory leak Add ISL data structures Add scop->context add pbb->domain add pdr->accesses and pdr->extent ChangeLog | 17 ++ config/cloog.m4 | 109 +------- configure | 176 +------------ configure.ac | 2 +- gcc/ChangeLog | 18 ++ gcc/Makefile.in | 4 +- gcc/doc/install.texi | 24 +-- gcc/graphite-blocking.c | 12 +- gcc/graphite-clast-to-gimple.c | 414 +++++++++++++----------------- gcc/graphite-clast-to-gimple.h | 1 - gcc/graphite-cloog-compat.h | 275 -------------------- gcc/graphite-cloog-util.c | 37 ++-- gcc/graphite-cloog-util.h | 3 - gcc/graphite-dependences.c | 11 +- gcc/graphite-flattening.c | 11 +- gcc/graphite-interchange.c | 12 +- gcc/graphite-poly.c | 39 +++- gcc/graphite-poly.h | 53 +++-- gcc/graphite-ppl.c | 11 +- gcc/graphite-scop-detection.c | 11 +- gcc/graphite-sese-to-poly.c | 552 +++++++++++++++++++++++++++++++++++++--- gcc/graphite.c | 18 +- 22 files changed, 938 insertions(+), 872 deletions(-) delete mode 100644 gcc/graphite-cloog-compat.h
From 872bd33a1967d7a3dc00330f56d073f2a9f6fc13 Mon Sep 17 00:00:00 2001 From: Sebastian Pop <sebpop@gmail.com> Date: Wed, 10 Aug 2011 13:08:37 -0500 Subject: [PATCH 10/10] add pdr->accesses and pdr->extent --- gcc/graphite-cloog-util.c | 24 +++++++ gcc/graphite-cloog-util.h | 2 + gcc/graphite-poly.c | 7 ++- gcc/graphite-poly.h | 4 +- gcc/graphite-sese-to-poly.c | 161 ++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 186 insertions(+), 12 deletions(-) diff --git a/gcc/graphite-cloog-util.c b/gcc/graphite-cloog-util.c index c5abde8..8d46acb 100644 --- a/gcc/graphite-cloog-util.c +++ b/gcc/graphite-cloog-util.c @@ -428,6 +428,18 @@ debug_isl_set (isl_set *s) isl_ctx_free (ctx); } +/* 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); +} + /* Prints an isl_pw_aff A to stderr. */ DEBUG_FUNCTION void @@ -452,4 +464,16 @@ debug_isl_aff (isl_aff *a) isl_ctx_free (ctx); } +/* Prints an isl_id I to stderr. */ + +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); +} + #endif diff --git a/gcc/graphite-cloog-util.h b/gcc/graphite-cloog-util.h index 3d10845..a72ff6c 100644 --- a/gcc/graphite-cloog-util.h +++ b/gcc/graphite-cloog-util.h @@ -36,8 +36,10 @@ void openscop_read_polyhedron_matrix (FILE *, ppl_Polyhedron_t *, int *, int *, extern int *openscop_read_N_int (FILE *, int); void debug_isl_set (isl_set *); +void debug_isl_map (isl_map *); void debug_isl_pwaff (isl_pw_aff *); void debug_isl_aff (isl_aff *); +void debug_isl_id (isl_id *); static inline bool isl_set_is_equal_ppl_polyhedron (isl_set *s1, ppl_const_Polyhedron_t ph, diff --git a/gcc/graphite-poly.c b/gcc/graphite-poly.c index c5b32d6..c084634 100644 --- a/gcc/graphite-poly.c +++ b/gcc/graphite-poly.c @@ -847,7 +847,8 @@ pbb_remove_duplicate_pdrs (poly_bb_p pbb) void new_poly_dr (poly_bb_p pbb, int dr_base_object_set, ppl_Pointset_Powerset_C_Polyhedron_t accesses, - enum poly_dr_type type, void *cdr, graphite_dim_t nb_subscripts) + enum poly_dr_type type, void *cdr, graphite_dim_t nb_subscripts, + isl_map *acc, isl_set *extent) { static int id = 0; poly_dr_p pdr = XNEW (struct poly_dr); @@ -857,6 +858,8 @@ new_poly_dr (poly_bb_p pbb, int dr_base_object_set, PDR_NB_REFS (pdr) = 1; PDR_PBB (pdr) = pbb; PDR_ACCESSES (pdr) = accesses; + pdr->accesses = acc; + pdr->extent = extent; PDR_TYPE (pdr) = type; PDR_CDR (pdr) = cdr; PDR_NB_SUBSCRIPTS (pdr) = nb_subscripts; @@ -869,6 +872,8 @@ void free_poly_dr (poly_dr_p pdr) { ppl_delete_Pointset_Powerset_C_Polyhedron (PDR_ACCESSES (pdr)); + isl_map_free (pdr->accesses); + isl_set_free (pdr->extent); XDELETE (pdr); } diff --git a/gcc/graphite-poly.h b/gcc/graphite-poly.h index 0ca46ab..019f99f 100644 --- a/gcc/graphite-poly.h +++ b/gcc/graphite-poly.h @@ -182,6 +182,7 @@ struct poly_dr In the example, the vector "R C O I L P" is "7 7 3 2 0 1". */ ppl_Pointset_Powerset_C_Polyhedron_t _accesses; isl_map *accesses; + isl_set *extent; /* Data reference's base object set number, we must assure 2 pdrs are in the same base object set before dependency checking. */ @@ -201,7 +202,8 @@ struct poly_dr #define PDR_NB_SUBSCRIPTS(PDR) (PDR->nb_subscripts) void new_poly_dr (poly_bb_p, int, ppl_Pointset_Powerset_C_Polyhedron_t, - enum poly_dr_type, void *, graphite_dim_t); + enum poly_dr_type, void *, graphite_dim_t, isl_map *, + isl_set *); void free_poly_dr (poly_dr_p); void debug_pdr (poly_dr_p, int); void print_pdr (FILE *, poly_dr_p, int); diff --git a/gcc/graphite-sese-to-poly.c b/gcc/graphite-sese-to-poly.c index dc1fad8..1d31392 100644 --- a/gcc/graphite-sese-to-poly.c +++ b/gcc/graphite-sese-to-poly.c @@ -670,6 +670,27 @@ isl_id_for_loop (scop_p s, loop_p l) return id; } +/* Return an ISL identifier for the data reference DR. */ + +static isl_id * +isl_id_for_dr (scop_p s, data_reference_p dr) +{ + isl_id *id; + const char *name = get_name (DR_BASE_OBJECT (dr)); + + if (name) + id = isl_id_alloc (s->ctx, name, dr); + else + { + char *name1 = XNEWVEC (char, 1); + sprintf (name1, "A"); + id = isl_id_alloc (s->ctx, name1, dr); + XDELETEVEC (name1); + } + + return id; +} + /* Extract an affine expression from the ssa_name E. */ static isl_pw_aff * @@ -1912,8 +1933,9 @@ build_scop_iteration_domain (scop_p scop) ACCESSES polyhedron, DOM_NB_DIMS is the dimension of the iteration domain. */ -static void -pdr_add_alias_set (ppl_Polyhedron_t accesses, data_reference_p dr, +static isl_map * +pdr_add_alias_set (isl_map *acc, + ppl_Polyhedron_t accesses, data_reference_p dr, ppl_dimension_type accessp_nb_dims, ppl_dimension_type dom_nb_dims) { @@ -1934,6 +1956,37 @@ pdr_add_alias_set (ppl_Polyhedron_t accesses, data_reference_p dr, ppl_delete_Linear_Expression (alias); ppl_delete_Constraint (cstr); + + { + isl_constraint *c = isl_equality_alloc (isl_map_get_dim (acc)); + c = isl_constraint_set_constant_si (c, -alias_set_num); + c = isl_constraint_set_coefficient_si (c, isl_dim_out, 0, 1); + + return isl_map_add_constraint (acc, c); + } +} + +/* Assign the affine expression INDEX to the output dimension POS of + MAP and return the result. */ + +static isl_map * +set_index (isl_map *map, int pos, isl_pw_aff *index) +{ + isl_map *index_map; + int len = isl_map_dim (map, isl_dim_out); + isl_id *id; + + index_map = isl_map_from_pw_aff (index); + + /* FIXME: This function will be renamed isl_map_insert_dims and + documented in a later version of ISL (current ISL is 0.07). */ + index_map = isl_map_insert (index_map, isl_dim_out, 0, pos); + index_map = isl_map_add_dims (index_map, isl_dim_out, len - pos - 1); + + id = isl_map_get_tuple_id (map, isl_dim_out); + index_map = isl_map_set_tuple_id (index_map, isl_dim_out, id); + + return isl_map_intersect (map, index_map); } /* Add to ACCESSES polyhedron equalities defining the access functions @@ -1941,8 +1994,9 @@ pdr_add_alias_set (ppl_Polyhedron_t accesses, data_reference_p dr, polyhedron, DOM_NB_DIMS is the dimension of the iteration domain. PBB is the poly_bb_p that contains the data reference DR. */ -static void -pdr_add_memory_accesses (ppl_Polyhedron_t accesses, data_reference_p dr, +static isl_map * +pdr_add_memory_accesses (isl_map *acc, + ppl_Polyhedron_t accesses, data_reference_p dr, ppl_dimension_type accessp_nb_dims, ppl_dimension_type dom_nb_dims, poly_bb_p pbb) @@ -1975,9 +2029,13 @@ pdr_add_memory_accesses (ppl_Polyhedron_t accesses, data_reference_p dr, ppl_delete_Linear_Expression (fn); ppl_delete_Linear_Expression (access); ppl_delete_Constraint (cstr); + + acc = set_index (acc, i + 1, extract_affine (scop, afn)); } mpz_clear (v); + + return acc; } /* Add constrains representing the size of the accessed data to the @@ -1985,8 +2043,9 @@ pdr_add_memory_accesses (ppl_Polyhedron_t accesses, data_reference_p dr, ACCESSES polyhedron, DOM_NB_DIMS is the dimension of the iteration domain. */ -static void -pdr_add_data_dimensions (ppl_Polyhedron_t accesses, data_reference_p dr, +static isl_set * +pdr_add_data_dimensions (isl_set *extent, scop_p scop, + ppl_Polyhedron_t accesses, data_reference_p dr, ppl_dimension_type accessp_nb_dims, ppl_dimension_type dom_nb_dims) { @@ -2024,6 +2083,52 @@ pdr_add_data_dimensions (ppl_Polyhedron_t accesses, data_reference_p dr, high = array_ref_up_bound (ref); + 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_id *id; + isl_aff *aff; + isl_set *univ, *lbs, *ubs; + isl_pw_aff *index; + isl_dim *dim; + isl_set *valid; + int nbl = isl_set_dim (scop->context, isl_dim_set); + int nbs = isl_set_dim (extent, isl_dim_set); + isl_pw_aff *lb = extract_affine_int (scop, low); + isl_pw_aff *ub = extract_affine_int (scop, high); + + /* high >= 0 */ + valid = isl_pw_aff_nonneg_set (isl_pw_aff_copy (ub)); + scop->context = isl_set_intersect (scop->context, valid); + + /* 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); + + dim = isl_set_get_dim (extent); + aff = isl_aff_zero (isl_local_space_from_dim (dim)); + aff = isl_aff_add_coefficient_si (aff, isl_dim_set, i, 1); + univ = isl_set_universe (isl_aff_get_dim (aff)); + index = isl_pw_aff_alloc (univ, aff); + + id = isl_set_get_tuple_id (extent); + lb = isl_pw_aff_set_tuple_id (lb, isl_id_copy (id)); + ub = isl_pw_aff_set_tuple_id (ub, id); + + /* low <= sub_i <= high */ + lbs = isl_pw_aff_ge_set (isl_pw_aff_copy (index), lb); + ubs = isl_pw_aff_le_set (index, ub); + extent = isl_set_intersect (extent, lbs); + extent = isl_set_intersect (extent, ubs); + } + /* high - subscript >= 0 */ if (high && host_integerp (high, 0) /* 1-element arrays at end of structures may extend over @@ -2042,6 +2147,8 @@ pdr_add_data_dimensions (ppl_Polyhedron_t accesses, data_reference_p dr, ppl_delete_Constraint (cstr); } } + + return extent; } /* Build data accesses for DR in PBB. */ @@ -2054,6 +2161,9 @@ build_poly_dr (data_reference_p dr, poly_bb_p pbb) ppl_dimension_type dom_nb_dims; ppl_dimension_type accessp_nb_dims; int dr_base_object_set; + isl_map *acc; + isl_set *extent; + scop_p scop = PBB_SCOP (pbb); ppl_Pointset_Powerset_C_Polyhedron_space_dimension (PBB_DOMAIN (pbb), &dom_nb_dims); @@ -2062,9 +2172,40 @@ build_poly_dr (data_reference_p dr, poly_bb_p pbb) ppl_new_C_Polyhedron_from_space_dimension (&accesses, accessp_nb_dims, 0); - pdr_add_alias_set (accesses, dr, accessp_nb_dims, dom_nb_dims); - pdr_add_memory_accesses (accesses, dr, accessp_nb_dims, dom_nb_dims, pbb); - pdr_add_data_dimensions (accesses, dr, accessp_nb_dims, dom_nb_dims); + { + 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)); + + acc = isl_map_universe (dim); + acc = isl_map_set_tuple_id (acc, isl_dim_out, isl_id_for_dr (scop, dr)); + isl_dim_free (dc); + } + + acc = pdr_add_alias_set (acc, accesses, dr, accessp_nb_dims, dom_nb_dims); + acc = pdr_add_memory_accesses (acc, accesses, dr, accessp_nb_dims, + dom_nb_dims, pbb); + + { + isl_id *id = isl_id_for_dr (scop, dr); + int nb = 1 + DR_NUM_DIMENSIONS (dr); + isl_dim *dim = isl_dim_set_alloc (scop->ctx, 0, nb); + + dim = isl_dim_set_tuple_id (dim, isl_dim_set, id); + extent = isl_set_nat_universe (dim); + extent = pdr_add_data_dimensions (extent, scop, accesses, dr, + accessp_nb_dims, dom_nb_dims); + } ppl_new_Pointset_Powerset_C_Polyhedron_from_C_Polyhedron (&accesses_ps, accesses); @@ -2075,7 +2216,7 @@ build_poly_dr (data_reference_p dr, poly_bb_p pbb) new_poly_dr (pbb, dr_base_object_set, accesses_ps, DR_IS_READ (dr) ? PDR_READ : PDR_WRITE, - dr, DR_NUM_DIMENSIONS (dr)); + dr, DR_NUM_DIMENSIONS (dr), acc, extent); } /* Write to FILE the alias graph of data references in DIMACS format. */ -- 1.7.4.1