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
