Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to construct isl sets. #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tobiasgrosser
Copy link
Member

@tobiasgrosser tobiasgrosser commented May 3, 2018

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
Copy link

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?

@tobiasgrosser tobiasgrosser force-pushed the constructing-a-set branch 4 times, most recently from e797d67 to d595b23 Compare May 4, 2018 10:54
@0152la
Copy link

0152la commented May 4, 2018

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
Copy link

pfaffe commented May 4, 2018

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.

README.md Outdated
## Constructing an integer set or map (isl::set / isl::map)

We create an integer set as follows. We first create a set of identifiers for
each of the needed dimensions (`isl::id Id_N(ctx, "N"`). We then introduce
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

README.md Outdated
isl::id Id_N(ctx, "N"), Id_M(ctx, "M"), Id_i(ctx, "i"), Id_j(ctx, "j");

// One (piece-wise) affine expression per identifier
isl::pw_aff N(Id_N), M(Id_M), i(Id_i), j(Id_j);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

README.md Outdated
isl::pw_aff N(Id_N), M(Id_M), i(Id_i), j(Id_j);

// One (piece-wise) affine expression per constant
isl::pw_aff Ten(ctx, 10), Two(ctx, 2), Three(ctx, 3);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.md Outdated
isl::pw_aff Ten(ctx, 10), Two(ctx, 2), Three(ctx, 3);

// Build the left and right hand side of the expression
isl::pw_aff LHS = Two.mul(M).add(Three.mul(N));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@pfaffe pfaffe May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

README.md Outdated
// One (piece-wise) affine expression per identifier
isl::pw_aff N(ctx, "N"), M(ctx, "M"), i(ctx, i"), j(ctx, "j");
```
This likely won't work as there is no way to obtain ids for the identifiers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

actually, this

obtain ids for the identifiers.

is confusing

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that implemented right now?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ftynse
Copy link
Member

ftynse commented May 4, 2018

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
Copy link

pfaffe commented May 4, 2018

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
Copy link

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

@nicolasvasilache
Copy link

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

+10e9 for overloads

@ftynse
Copy link
Member

ftynse commented May 4, 2018

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
Copy link
Member

ftynse commented May 4, 2018

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
Copy link
Member

ftynse commented May 4, 2018

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
Copy link
Member

ftynse commented May 4, 2018

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

@tobiasgrosser tobiasgrosser force-pushed the constructing-a-set branch 9 times, most recently from 9c02918 to 5f734c2 Compare May 5, 2018 08:07
@tobiasgrosser
Copy link
Member Author

@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
Copy link
Member Author

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

@tobiasgrosser
Copy link
Member Author

@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
Copy link

pfaffe commented May 5, 2018

@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
Copy link
Member

ftynse commented May 5, 2018

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
Copy link
Member

ftynse commented May 5, 2018

@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
Copy link
Member Author

tobiasgrosser commented May 5, 2018

@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
Copy link
Member Author

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

@tobiasgrosser tobiasgrosser force-pushed the constructing-a-set branch 2 times, most recently from 1ac114e to 8ef76ee Compare May 5, 2018 11:35
@tobiasgrosser
Copy link
Member Author

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

@tobiasgrosser tobiasgrosser force-pushed the constructing-a-set branch 4 times, most recently from d91c31d to 2748ab7 Compare May 5, 2018 20:28
@pfaffe
Copy link

pfaffe commented May 6, 2018

@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
Copy link
Member Author

tobiasgrosser commented May 6, 2018

@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
Copy link

pfaffe commented May 7, 2018

@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
Copy link
Member Author

tobiasgrosser commented May 16, 2018

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:

@tobiasgrosser tobiasgrosser force-pushed the constructing-a-set branch 6 times, most recently from f635c2f to 5646a53 Compare May 16, 2018 21:21
@tobiasgrosser
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants