How to construct isl sets.

[tobiasgrosser]

On today's Polly Labs call, we discussed the idea of creating better documentation how to use the new isl interfaces. This is a first proposal of how to create an isl::set.

[albertcohen]

Sounds like a good move indeed. The text can probably be polished a bit more, with shorter sentences, but it already makes a lot of sense, pedagogically speaking.
Regarding the interface proposals at the bottom, they seem to somewhat conflict: you don't define Id_i anymore with the shortcut "i" pw_aff construction, but need it in the follow-up set construction. Any idea how to reconcile these?

[0152la]

The example is really good, but I feel it would be better by making the relation between the text and the example clearer (e.g. the parametric set is constructed via isl::set ParameterSet = LHS.le_set(RHS);). Perhaps it would be better to interleave the two?

[pfaffe]

I like the idea. A few comments on the API:

• I'm not opposed to creating the identifiers. Those are what you often have at hand anyways. Instead, why not skip the creation of the pw_affs? If we overload arithmetic operators this can just become:
  isl::pw_aff LHS = 2 * Id_M + 3 * Id_N;. This requires implicitely converting IDs to pw_affs, and overloading the operations for constants.
• LHS.le_set(RHS) IMO should be more elementary. It should be LHS.le(RHS), which is of type constraint. Sets should be constructible from constraints, optionally also with a sequence of IDs to make set dimensions.
• Mixing set construction with ID sequences could end up dangerous and confusing: isl::set Set({Id_i, Id_j}, PSet}). What if PSet already has set dimensions? I mean, you can make it work by demoting unspecified set dimensions to param dimensions, or some other hack, but then reading the code becomes pure guesswork of what will actually happen. I think set and map construction should be very explicit. They are a compound of input dimensions, possibly output dimensions, and constraints. So just build them like that.

[ftynse]

This seems like the most annoying part (and you also commented on this below). Given that our current take on exporting ids is to ignore the user pointers in the C++/Python, maybe we can assume a bijection between names and isl::id objects and just use string literals instead of ids in context where isl::ctx object is available.

[pfaffe]

I don't think this will pay off actually. Id identity would then mean string value identity, instead of the reference identity we have now. Names are not generally literals, but could come from std::string or const char* or anything that resembles a string.

[ftynse]

I meant something like

auto A = isl::aff(isl::id(ctx, "A"));
auto AplusB = A.plus("B");

but it's not something that looks easy to get right from the first take.

[pfaffe]

Sure, but this would require string compare to find the proper Id, and isl is already not the fastest library on the planet :)

[ftynse]

It wouldn't. When you construct an id with the same name, it is guaranteed to be the same id. Internally, it's a hash map based on name and user pointer (always null from C++) AFAIR.

[albertcohen]

Yes, given that the user pointer is always null with the C++ interface, it should work with plain pointer ==
It will not work with hybrid C and C++ code however, but such hybrid schemes are unlikely to cause trouble in set/map construction. And also the user pointer may end up being deprecated in the C world as well.
Still, I like Philip's idea of implicit lifting of IDs into pw_aff.

[tobiasgrosser]

Right, I think constructing [pw_]affs from strings is easy. Where I don't feel there is a good way is for extracting an id from a pw_aff that has already been constructed. The reason is that even though the pw_aff just contains a single id when we constructed, an affine expression can obviously have multiple dimensions, so we cannot just add a function aff::get_id.

@pfaffe's idea of allowing conversion from id: to aff solves this problem nicely. However, it would mean we would need to provide operators which operations between ints , ids, affs, pw_affs, ... I feel this could make the interface a lot nicer, but we need to be careful how much operator overloading we actually allow in the end in terms of optimal clarity.

[nicolasvasilache]

Just my 2 cents, take with a grain of salt :)

re context where isl::ctx object is available.

Is there concretely a benefit in manipulating more than 1 threadlocal context at a time?
Could this be better captured by a global threadlocal context (the context) with maybe threadsafe RAII-style scopes like WithDevice for CUDA, like we do here.

I think I remember there were arguments about handing off objects from one context to another in the case of multithreading but I am not sure. If there are such considerations, I would hope people plan to use modern threading concepts (C++11 onwards).

In any case, to be more concrete about the API / examples proposed here, I feel exposing ctx to all users is an abstaction leak. I would prefer things in a detail namespace where one must pass ctx and a more vanilla API

re: ref vs string id

FWIW Halide's take on this is to use strings as ids and it simplifies code greatly.
There were a bunch of rewrites in TC that Sven did for us because one could not rely on the name of the ID. I do not fully understand the underlying issues here but they seemed somewhat artificial to me.

How much of this comes from not using a simple unordered_map in the entrails of ISL?

string compare to find the proper Id, and isl is already not the fastest library on the planet

Hashing should solve this. In any case, seems to me that worrying about strcmp in a word where we routinely run ILPs in a single thread is premature optimization :)

Where I don't feel there is a good way is for extracting an id from a pw_aff that has already been constructed

string == id solves that but of course it has to work with the entrails of ISL

Generally, I am wondering how much of the ISL interface quirks comes from the fact that it's written in C where STL is unavailable. As a consequence, are the C++ bindings the right abstraction for a user? In the same way operator overloading is useful I am wondering whether simple high-level wrappers could hide some of the complexity away.

[ftynse]

What is the space of these expressions? Assuming an implicitly constructed parameter space containing only the given parameter.

[ftynse]

I wouldn't use a pw aff for something that is not actually piece-wise

[tobiasgrosser]

The space is a parameter space with a single dimension.

Right now we need to use pw_aff as aff does not support auto-alignment of parameter dimensions. This should be improved eventually, but -- as Sven said -- this requires more work.

[nicolasvasilache]

Right now we need to use pw_aff as aff does not support auto-alignment

these type of concerns are exactly the reason why I wonder if the current C++ bindings are the right abstraction or whether we could have some simple higher level wrapper that may only expose a subset of ISL but that are much more natural in C++.

In other words a general comment I have here is that the design is bottom-up, which is very important to connect to reality and a working impl. But we should also maybe think about top down: how would we write these things ideally and can we implement them naturally with ISL.

Rinse, repeat.

[ftynse]

What is the space of these expressions? An implicitly constructed empty space?

[tobiasgrosser]

An implicitly constructed space with 0 param, 0 set, and 0 output dimensions.

[ftynse]

Here is were overloaded operators come in handy. We can overload operators to combine pw_aff, integers and val, which would remove the need to create "constant" affs Ten, Two, Three. If we proceed carefully, we can also overload operators betwee ids and integers so that they return an aff.

[albertcohen]

Absolutely. The question I have at this point is, when do you know a constant/parameter should be lifted into an aff or a pw_aff?

[pfaffe]

Since a single parameter or constant is unconstrained, it should be an aff.

[tobiasgrosser]

Right.

[ftynse]

We can construct a new id, identifiers with identical names are guaranteed to be identical if all of them are constructed from C++.

[pfaffe]

I'm not sure I get what you mean by identical here.

For this example, we can just write isl::pw_aff N({ctx, "N"}), M({ctx, "M"}), i({ctx, i"}), j({ctx, "j"}); which is only negligibly more verbose.

[ftynse]

isl::id id1(ctx, "A");
isl::id id2(ctx, "A");
id1 == id2; // must be true

actually, this

obtain ids for the identifiers.

is confusing

[pfaffe]

Is that implemented right now?

[pfaffe]

Apparently it is, I had no idea. I'm used to Polly's version of the interface, where the user pointer is present.

[ftynse]

Not sure about the operator== overload, but construction is implemented.

[ftynse]

Generally, I like the idea. There are some syntactic improvements that I could propose, but it depends on the flavor of C++ that people prefer. Operator overloads would significantly improve readability here, as well as constructing affs from names instead of ids.

isl::aff N(ctx, "N"), M(ctx, "M"), i(ctx, "i"), j(ctx, "j');
auto LHS = 2 * M + 3 * N;  // natural precedence works
auto RHS = 2 * i + j + 10;
auto PSet = LHS.le_set(RHS);
// { [N, M] -> { [i,j] : 2 * M + 3 * N <= 2 * i + j + 10 }
auto Set = PSet.inputs(i, j);
// { [N, M] -> { [i]->[j] : 2 * M + 3 * N <= 2 * i + j + 10 }
auto Map = PSet.inputs(i).outputs(j);

We can overload operators when one of the arguments is already a (pw)aff so as to implicitly construct an aff for another argument in the same space (or defined on the same domain for pw_aff), that's why stuff like 2 * M could work. The part for aff is already working in TC, see https://github.com/facebookresearch/TensorComprehensions/blob/master/tc/external/detail/islpp-inl.h#L32.

Additionally, we can have a marker type and overload comparison operators for parametric set construction.

auto PSet = isl::set_maker(LHS) <= RHS;
// or directly
auto PSet = isl::set_maker(2 * M + 3 * N) <= 2 * i + j + 10;

I suggest marker types because a non-boolean result of the <= operator may be confusing and a good enough name can help understand what happens there. We have a similar mechanism in TC https://github.com/facebookresearch/TensorComprehensions/blob/master/tc/external/detail/islpp-inl.h#L77, but we use markers to differentiate between map and set construction. This is necessary in our case because our affs live in non-parametric spaces.

[pfaffe]

Im not sure I agree with the marker approach.

How is this
auto PSet = isl::set_maker(LHS) <= RHS;
more readable than
isl::constraint PSet = LHS <= RHS;

[nicolasvasilache]

Sounds like a great endeavor to me, thanks for starting this!

[nicolasvasilache]

Operator overloads would significantly improve readability here, as well as constructing affs from names instead of ids

+10e9 for overloads

[ftynse]

It will not work with hybrid C and C++ code however

Mixing C and C++ API is already discouraged. And it seems there will be other functionality available only in C...

[ftynse]

Where I don't feel there is a good way is for extracting an id from a pw_aff that has already been constructed.

Yes, this seems indeed problematic with the current functionality. I think we are missing some property inspection functions, but this can go to a separate use case. In particular, I would like to know if a (pw)aff is constant everywhere defined, or is a variable/parameter everywhere defined. There is a function for the first part in aff, but for the second, I would go over coefficients now...

[ftynse]

How is this
auto PSet = isl::set_maker(LHS) <= RHS;
more readable than
isl::constraint PSet = LHS <= RHS;

That's why I said it depends on the flavor of C++ you prefer.

I would argue that it is more readable then LHS.le_set(RHS), which is what we had originally. Now, I would not specify a type for PSet there and end up writing
auto PSet = LHS <= RHS;.
When skimming through the code, one would almost certainly assume typeof(PSet) = bool because there is a comparison operator! While it is possible to avoid using auto in this case and type out explicitly, it is a non-enforcible recommendation that should be picked up in, e.g., code reviews. Having comparison operators overloaded for marker types only forces you to spell the type out at least once (either constructing a named object auto lhsMaker = isl::set_maker(LHS) or a temporary like in my original example), lest you have a compilation error.

Additional motivation, overloading comparison operators to not-default semantics (that is not comparison) may lead to some difficult to track errors. For example, one could call std::sort on a list of aff because operator < (and hence std::less) is defined, but there is no order defined on affs so sorting does not have sense. In the best case, you get a couple of screens of template instantiation errors because operator < does not return bool. In a worse case, we are considering isl types to be convertible to bool to check the underlying pointer for NULL Given isl::aff a; if (a.get()) {} can be made equivalent to if (a) {} (this is already the case in TC's version of isl, but will be discussed for the mainline). In this case, if (LHS <= RHS) becomes non-obvious to interpret as it would check the constructed set for non-null instead of what is written. Finally, what should be done with eq_set? I personally would expect LHS == RHS to be equivalent to LHS.is_equal(RHS), rather than to LHS.eq_set(RHS). Spurious semantics can be avoided by allowing only one comparison operator argument to have the marker type: thus marker type objects are not comparable to each other, and neither are affs.

[ftynse]

I don't seem to be able to request reviews on this PR, but please add @skimo-openhub

[tobiasgrosser]

@ALL, thanks for participating in the discussion. I tried to summarize and structure the feedback so far.

The one item I did not yet summarize well was the point "Return type of the comparision operator". I did not understand the marker stuff well enough to write a good summary. @ftynse, maybe you want expand here?

The problem with Philip's proposal is that an isl::constraint really just is a single constraint, whereas at least isl::pw_affs can be set of constraints, so the actual element that is returned must be a set of constraints (an isl::set).

[tobiasgrosser]

@ftynse, Sven is not a member of the PollyLabs github organization. I invited him now.

[tobiasgrosser]

@alex: You should now also have all rights needed to invite and change things in the PollyLabs repos. Please check if adding reviewers works now.

[pfaffe]

@TobiG could you clarify for me in what way a pw_aff might induce a set of constraints? Every piece becomes a single constraint? We should clarify if we want to have pw_aff's or aff's as smallest building blocks here. From the discussion and the example, it looks to me like currently all the spaces involved are universe, and so there is no real way to produce any distinct pieces yet.

@ftynse Looks like it comes down to a matter of taste. I dislike the markers because they trip up my reading if i'm familiar with what they mean. If I'm not, I'm forced to go back to the documentation to understand that this is non-optional syntactic sugar. It doesn't feel "natural" to me to write it this way. Regarding your second argument, std::less only has an issue with operator<, since isl objects convert to bool, which is just madness and should be deprecated or demolished.

[ftynse]

it looks to me like currently all the spaces involved are universe, and so there is no real way to produce any distinct pieces yet.

Indeed, we should discuss (separately) how do we construct something like {[i,j]->[k]: (i < j and k = 42) or (i >= j and k = 0)}. So far, we can only construct sets with one constraint only.

Looks like it comes down to a matter of taste.

Yep, that's what I started my proposition with ;)

It doesn't feel "natural" to me to write it this way.

It is not supposed to be natural, it is supposed to force you to read the documentation or trip up on the non-optional syntactic sugar to understand what happens. I consider it an instance of "recognition over recall" UI design principle (and API design an instance of UI design). You recognize the markers rather than recall the types of LHS and RHS and that comparison operators on those do not mean comparison. However, the cases that we've seen so far are short so you probably won't forget the types before you need them.

I'm not sure this how to implement this and trump ADL, but something like

auto LHS = /*...*/;
auto RHS = /*...*/;
using namespace isl::set_construction;
auto PSet = LHS <= RHS;

where construction-by-comparison operators are defined in the set_construction namespace and are not picked up unless explicitly requested may better.

since isl objects convert to bool, which is just madness and should be deprecated or demolished.

They are not yet convertible in the mainline AFAIR. That will be up for a separate discussion.

[ftynse]

@pfaffe what is your take on opeator== wrt to .eq_set and .is_equal ?

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ro-conventional

[tobiasgrosser]

@pfaffe: An isl_aff indeed can be translated into a single basic_set and (if I am not mistaken) into a single constraint. So yes, we could add a w where we to translate isl_aff's to constraints. However, there are operations on isl_pw_affs that are useful (e.g., a ? b : c isl_pw_aff_cond, or division with rounding towards zero isl_pw_aff_tdiv_q), which are very useful and will result in more than one constraint. I would prefer to have an interface that is consistent even if we use some of this functionality.

[tobiasgrosser]

@ftynse : I like the idea with having some of the operators in separate name spaces.

[tobiasgrosser]

@pfaffe, I am happy for us to drop the conversion to bool stuff from polly. I don't feel it's important.

[pfaffe]

@ftynse I agree with you that operator== should be is_equal. But maybe, if we do the namespace trick, that won't be so bad? The only limitation with using an operator namespace is that this doesn't work with templates. So, anything like std::less or std::is_equal won't work, but I think that's ok.

@TobiG Can you make an example of how you'd want to write an expression which needs to be represented as a pw_aff?

[tobiasgrosser]

@pfaffe: Today the following functions require pw_aff

isl::aff A("a"), B("42");
// [a] -> { [floor(a/42)] : a >= 0; [floor((a+41)/42)] : a < 0}
isl::pw_aff AfterRoundToZeroDiv = isl::pw_aff(A).tdiv_q(B);

isl::aff C("c"), D("d"), E("e");
// [c,d,e] -> { [(d)] : c != 0; [(e)] : c == 0}
isl::pw_aff IfThanElse = isl::pw_aff(C).cond(D, E);

[pfaffe]

@TobiG Fair enough. Are those an example against using constraints/affs as basic building blocks though? This code seems to be a perfectly elegant solution to computing tdiv_q or cond, especially since there are no operators to implement them. If you wanted, you could implement them as free functions and benefit from automatic conversion.

[tobiasgrosser]

We discussed this topic again on the Polly phone call (Michael & Philip & Tobias & others). Between ourselves we felt that we should:

• add functions that allow to convert affs to constraints (e.g., isl_aff_le_constraint)
• provide an example where isl::aff is used instead of pw_aff (but don't make it the default as isl::aff currently does not auto-align paramters).
• provide patches that complete the first example.
• we agree with Oleksandre's proposal of making operators optional in a separate name space.

I will update the document, here the pull requests that complete the proposal's initial interface:

• Simplify construction of parameteric pw affs isl#25
• Construct set and map from parameter set and isl_id lists isl#26

[tobiasgrosser]

I just updated the proposal. Please feel invited to give feedback on proposal and patches.