This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Add C++ bindings (method interface)
ClosedPublic

Authored by grosser on Mar 5 2017, 8:15 AM.

Details

Summary

The isl C++ binding method interface introduces a thin C++ layer that allows
to call isl methods directly on the memory managed C++ objects. This makes the
relevant methods directly available via code-completion interfaces, allows for
the use of overloading, conversion constructors, and many other nice C++
features that make using isl a lot easier.

Diff Detail

Repository
rL LLVM

Event Timeline

grosser created this revision.Mar 5 2017, 8:15 AM
Meinersbur added inline comments.Mar 8 2017, 5:09 PM
lib/External/isl/include/isl-noexceptions.h
17–18 ↗(On Diff #90610)

What header guard name would the version with exceptions get? (Sorry, this should maybe go into D30325)

20–31 ↗(On Diff #90610)

Do we rely on some of the headers to be included implicitly (eg. isl/ctx.h)?

116–130 ↗(On Diff #90610)

For completeness: This is a non-owning ctx class, which IMHO is the right thing to do. Will we also get an owning version of it such that we can do:

{
isl::owning_ctx temporary;
auto map = isl::map(temporary,"{ [i] -> [2i] }");
...
}

a pattern very common in the unittests.

130 ↗(On Diff #90610)

isl_ctx also has functions such as isl_ctx_get_max_operations.

179 ↗(On Diff #90610)

The _val suffix is type-related which we usually prune from the method name. There's also no other get_constant.

180 ↗(On Diff #90610)

Returns bool, not boolean?

212–217 ↗(On Diff #90610)

Nice overloads

218 ↗(On Diff #90610)

This one stands out very differently. The dominant creation pattern is the constructor, not named static constructors.

I would generally prefer this. Eg. isl::basic_map::from_string(ctx,std::string) or isl::ctx::space(unsigned,unsigned,unsigned).

I don't think we had a discussion about it yet.

220 ↗(On Diff #90610)

A dump() method would be appreciated.

508–509 ↗(On Diff #90610)

isl_map_lexmin_pw_multi_aff seems to be not exposed.

780 ↗(On Diff #90610)

The method is called get_str, but the isl function is pw_multi_aff_to_str . Will Sven object?

982–983 ↗(On Diff #90610)

You had nice /* implicit */ markers somewhere else, but not here?

1317 ↗(On Diff #90610)

Why did isl_val_int_from_si become a constructor, but isl_val_int_from_ui just disappeared?

1399–1406 ↗(On Diff #90610)

How did you decide when to make a constructor? Is there a special case for zero_on_domain and val_on_domain?

1407 ↗(On Diff #90610)

const std::string &str

1668 ↗(On Diff #90610)

The char* returned by isl_ast_expr_to_C_str has to be free'd.

1779–1780 ↗(On Diff #90610)

Nit: Space between definitions missing

2384–2392 ↗(On Diff #90610)

For completeness: I'd prefer iterators over callback functions.

3710 ↗(On Diff #90610)

It would save one allocation if using isl_set_to_str directly instead of the get_str()

4514 ↗(On Diff #90610)

How did you decide when to make a constructor?

grosser marked 15 inline comments as done.Mar 10 2017, 12:29 AM

Hi Michael,

thank you for your comments. I replied to them inline and incorporated the necessary changes in the new patch, which I push in a second.

lib/External/isl/include/isl-noexceptions.h
17–18 ↗(On Diff #90610)

I renamed this (in D30325) to ISL_CPP_NOEXCEPTIONS.

20–31 ↗(On Diff #90610)

Yes. These are the files that were used to generate the bindings. They are known to be sufficient to provide all types in function signatures used in these bindings.

116–130 ↗(On Diff #90610)

I would like to get an owning class as well, but we can probably add it later. I will add the intentions to add such a class to the commit message that I propose to isl-dev.

130 ↗(On Diff #90610)

Sure, but there seems to be no need to expose them for now.

179 ↗(On Diff #90610)

Right. This function is only exposed in Polly. I would leave it for now as it is and submit a corresponding change to isl when exposing it officially.

180 ↗(On Diff #90610)

Changed.

212–217 ↗(On Diff #90610)

Thanks for the positive feedback!

218 ↗(On Diff #90610)

This function is annotated in isl as isl_export, not isl_constructor. Consequently I create a named constructor.
I tried to remain here as close as possible to what the python bindings do.

Are you saying you prefer to not have unnamed constructors, but generally use named constructors for exported isl methods? If you do, this seems to be a change of what Sven did in the python bindings. In this case, we should probably add this as a comment in the commit message and have a discussion with Sven involved.

220 ↗(On Diff #90610)

Added a dump method (Polly only for now).

508–509 ↗(On Diff #90610)

Right. It is also not exposed in isl. In fact, I will try to exclude them from the isl python bindings I submit for review and add a note that we discuss the right return type of these functions with Sven in the code review.

982–983 ↗(On Diff #90610)

Added in D30325.

1317 ↗(On Diff #90610)

isl_val_int_from_si is today already exposed by isl as unnamed constructor, whereas isl_val_int_from_ui is not exposed and I also did not expose it myself for Polly. I exposed it now for Polly as another unnamed constructor, such that we have now constructors for both.

1399–1406 ↗(On Diff #90610)

These are not exposed in isl at the moment, but I added the appropriate markers isl_polly_constructor myself. I believe these are simple enough that they might not need a named constructor, but I am happy to change this.

As these are not yet exposed in isl, I expect these not be part of the first bindings I submit. Hence, we can discuss these specific functions at the point when we submit a patch to expose these functions in isl.

1407 ↗(On Diff #90610)

String arguments are now passed as const std::string &str.

1668 ↗(On Diff #90610)

We now properly free the char* returned by *_to_C_str.

1779–1780 ↗(On Diff #90610)

Added spaces between constructors.

2384–2392 ↗(On Diff #90610)

I understand (and likely agree with this). I won't include these foreach functions in the patches to upstream.

3710 ↗(On Diff #90610)

Changed.

4514 ↗(On Diff #90610)

For the python bindings isl has specific markers __isl_constructor to decide which functions should become constructors. I translated precisely these to unnamed constructor and all others to named static functions. This matches the behavior of isl's current python bindings.

For functions not yet exposed, I marked what seemed obvious methods as constructors and left all others as named static functions.

grosser updated this revision to Diff 91276.Mar 10 2017, 12:30 AM
grosser marked 15 inline comments as done.

Address Michael's comments.

grosser updated this revision to Diff 91279.Mar 10 2017, 12:50 AM

Do not export a signed and unsigned constructor at the same time

grosser updated this revision to Diff 91284.Mar 10 2017, 1:18 AM

Make sure bindings are clang-format conforum

Meinersbur accepted this revision.Mar 10 2017, 4:01 AM
Meinersbur added inline comments.
lib/External/isl/include/isl-noexceptions.h
508–509 ↗(On Diff #90610)

I perceive isl_map_lexmin_pw_multi_aff as the more canonical variant. pw_multi_aff is implicitly convertible to map hence should make no difference in C++ if lexmin() returns pw_multi_aff instead of map.

Without isl_map_lexmin_pw_multi_aff, one has to use isl_pw_multi_aff_from_map as an extra step.

This revision is now accepted and ready to land.Mar 10 2017, 4:01 AM
This revision was automatically updated to reflect the committed changes.