This is an archive of the discontinued LLVM Phabricator instance.

Introduce isl C++ bindings, Part 1: value_ptr style interface
ClosedPublic

Authored by grosser on Feb 24 2017, 1:46 AM.

Details

Summary
Introduce isl C++ bindings, Part 1: value_ptr style interface

Over the last couple of months several authors of independent isl C++ bindings
worked together to jointly design an official set of isl C++ bindings which
combines their experience in developing isl C++ bindings. The new bindings have
been designed around a value pointer style interface and remove the need for
explicit pointer managenent and instead use C++ language features to manage isl
objects.

This commit introduces the smart-pointer part of the isl C++ bindings and
replaces the current IslPtr<T> classes, which served the very same purpose, but
had to be manually maintained. Instead, we now rely on automatically generated
classes for each isl object, which provide value_ptr semantics.

An isl object has the following smart pointer interface:

    inline set manage(__isl_take isl_set *ptr);

    class set {
      friend inline set manage(__isl_take isl_set *ptr);
      isl_set *ptr = nullptr;
      inline explicit set(__isl_take isl_set *ptr);

    public:
      inline set();
      inline set(const set &obj);
      inline set &operator=(set obj);
      inline ~set();
      inline __isl_give isl_set *copy() const &;
      inline __isl_give isl_set *copy() && = delete;
      inline __isl_keep isl_set *get() const;
      inline __isl_give isl_set *release();
      inline bool is_null() const;
    }

The interface and behavior of the new value pointer style classes is inspired
by http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3339.pdf, which
proposes a std::value_ptr, a smart pointer that applies value semantics to its
pointee.

We currently only provide a limited set of public constructors and instead
require provide a global overloaded type constructor method "isl::obj
isl::manage(isl_obj *)", which allows to convert an isl_set* to an isl::set by
calling 'S = isl::manage(s)'. This pattern models the make_unique() constructor
for unique pointers.

The next two functions isl::obj::get() and isl::obj::release() are taken
directly from the std::value_ptr proposal:

S.get() extracts the raw pointer of the object managed by S.
S.release() extracts the raw pointer of the object managed by S and sets the
object in S to null.

We additionally add std::obj::copy(). S.copy() returns a raw pointer refering
to a copy of S, which is a shortcut for "isl::obj(oldobj).release()", a
functionality commonly needed when interacting directly with the isl C
interface where all methods marked with __isl_take require consumable raw
pointers.

S.is_null() checks if S manages a pointer or if the managed object is currently
null. We add this function to provide a more explicit way to check if the
pointer is empty compared to a direct conversion to bool.

This commit also introduces a couple of polly-specific extensions that cover
features currently not handled by the official isl C++ bindings draft, but
which have been provided by IslPtr<T> and are consequently added to avoid code
churn. These extensions include:

        - operator bool() : Conversion from objects to bool
        - construction from nullptr_t
        - get_ctx() method
        - take/keep/give methods, which match the currently used naming
          convention of IslPtr<T> in Polly. They just forward to
          (release/get/manage).
        - raw_ostream printers

We expect that these extensions are over time either removed or upstreamed to
the official isl bindings.

We also export a couple of classes that have not yet been exported in isl (e.g.,
isl::space)

As part of the code review, the following two questions were asked:

- Why do we not use a standard smart pointer?

std::value_ptr was a proposal that has not been accepted. It is consequently
not available in the standard library. Even if it would be available, we want
to expand this interface with a complete method interface that is conveniently
available from each managed pointer. The most direct way to achieve this is to
generate a specialiced value style pointer class for each isl object type and
add any additional methods to this class. The relevant changes follow in
subsequent commits.

- Why do we not use templates or macros to avoid code duplication?

It is certainly possible to use templates or macros, but as this code is
auto-generated there is no need to make writing this code more efficient. Also,
most of these classes will be specialized with individual member functions in
subsequent commits, such that there will be little code reuse to exploit. Hence,
we decided to do so at the moment.

These bindings are not yet officially part of isl, but the draft is already very
stable. The smart pointer interface itself did not change since serveral months.
Adding this code to Polly is against our normal policy of only importing
official isl code. In this case however, we make an exception to showcase a
non-trivial use case of these bindings which should increase confidence in these
bindings and will help upstreaming them to isl.

Event Timeline

grosser created this revision.Feb 24 2017, 1:46 AM
grosser edited the summary of this revision. (Show Details)Feb 24 2017, 1:51 AM
Meinersbur added inline comments.Feb 24 2017, 4:08 AM
lib/External/isl/include/isl-noexceptions.h
830

inline explicit bool() const
Please add the explicit to avoid accidental conversions to int.

lib/Transform/FlattenSchedule.cpp
93

Didn't you add a ctor from nullptr_t?

grosser updated this revision to Diff 89644.Feb 24 2017, 4:18 AM

Address Michael's comments!

  • Make bool() explicit
  • Keep nullptr. We have explicit conversions.
grosser marked 2 inline comments as done.Feb 24 2017, 4:19 AM

I just realized that after adding 'explicit' to the boolean conversion, the unit tests in IslTest.cpp fail. It seems the operator overloading trick we used before does not work as the operator== is not called. Adding it back also does not help. The error I get is:

/home/grosser/Projects/polly/git/utils/unittest/googletest/include/gtest/gtest.h:1392:11: error: invalid operands to binary expression ('const isl::noexceptions::Map' and 'const isl::noexceptions::Map')
  if (lhs == rhs) {
      ~~~ ^  ~~~
/home/grosser/Projects/polly/git/utils/unittest/googletest/include/gtest/gtest.h:1421:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<isl::noexceptions::Map, isl::noexceptions::Map>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
/home/grosser/Projects/polly/git/tools/polly/unittests/Isl/IslTest.cpp:402:3: note: in instantiation of function template specialization 'testing::internal::EqHelper<false>::Compare<isl::noexceptions::Map, isl::noexceptions::Map>' requested here
  EXPECT_EQ(MAP("{ [] -> [i] : i <= 0 }"),
  ^
/home/grosser/Projects/polly/git/utils/unittest/googletest/include/gtest/gtest.h:1924:63: note: expanded from macro 'EXPECT_EQ'
                      EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \
                                                              ^
/home/grosser/Projects/polly/git/utils/unittest/googletest/include/gtest/internal/gtest-linked_ptr.h:223:6: note: candidate template ignored: could not match 'T *' against 'isl::noexceptions::Map'
bool operator==(T* ptr, const linked_ptr<T>& x) {

Michael, do you happen to have an idea what is going on here?

Meinersbur added inline comments.Feb 24 2017, 6:13 AM
unittests/Isl/IslTest.cpp
43–75

You removed these on purpose?

I did. They were marked as unused. Something fishy is going on here. Adding them back does not help.

Meinersbur edited edge metadata.Feb 24 2017, 6:30 AM

I did. They were marked as unused. Something fishy is going on here. Adding them back does not help.

This works for me:

namespace isl { namespace noexceptions {
static bool operator==(const Space &LHS, const Space &RHS) {
  auto IsEqual = isl_space_is_equal(LHS.keep(), RHS.keep());
  EXPECT_NE(isl_bool_error, IsEqual);
  return IsEqual;
}

static bool operator==(const Set &LHS, const Set &RHS) {
  auto IsEqual = isl_set_is_equal(LHS.keep(), RHS.keep());
  EXPECT_NE(isl_bool_error, IsEqual);
  return IsEqual;
}

static bool operator==(const Map &LHS, const Map &RHS) {
  auto IsEqual = isl_map_is_equal(LHS.keep(), RHS.keep());
  EXPECT_NE(isl_bool_error, IsEqual);
  return IsEqual;
}

static bool operator==(const UnionSet &LHS, const UnionSet &RHS) {
  auto IsEqual = isl_union_set_is_equal(LHS.keep(), RHS.keep());
  EXPECT_NE(isl_bool_error, IsEqual);
  return IsEqual;
}

static bool operator==(const UnionMap &LHS, const UnionMap &RHS) {
  auto IsEqual = isl_union_map_is_equal(LHS.keep(), RHS.keep());
  EXPECT_NE(isl_bool_error, IsEqual);
  return IsEqual;
}
}}

The reason might have to do with Koenig Lookup

grosser updated this revision to Diff 89669.Feb 24 2017, 7:29 AM

Add namespaces around operator== to make gtest work, as suggested by Michael.

Thanks, this works indeed.

grosser marked an inline comment as done.Feb 24 2017, 7:29 AM
Meinersbur added inline comments.Feb 24 2017, 7:35 AM
lib/Transform/FlattenSchedule.cpp
30

Did you remove the & on purpose?

grosser updated this revision to Diff 89671.Feb 24 2017, 7:38 AM

Add accidentally dropped '&'

S.copy() returns a raw pointer refering to a copy of S.

Why isn't this returning a managed pointer?

S.copy() returns a raw pointer refering to a copy of S.

Why isn't this returning a managed pointer?

Thanks for your interest.

The use of copy() is to interact with isl's C interface (and other code using raw pointers to isl objects). These functions take parameters annotated either with __isl_keep or __isl_take. __isl_take parameters assume to receive ownership of the isl object and will free it before they return. If the caller still needs the isl object it has to create a copy to it. This is what copy() does.

There is no use for copy() when using the C++ binding only. Simple assignment Dst = Src already copy the object implicitly (value-like semantics).

Btw, 'copy' does means increasing the reference counter, not a literal copy. The isl library considers this an implementation detail.

What big difference with std::unique_ptr (and a custom deleter) justify to rewrite so much code for your smart pointer?

There is a lot of code here that follow the same pattern.

I mean:

#define UniquePtr(Name, ISLType, Deleter)  \
   static auto  closer_##Name = [](ISLType* Ptr) {Deleter(Ptr); }; \
   using Name = std::unique_ptr<ISLType, decltype(closer_##Name);

UniquePtr(AstBuild, isl_ast_build, isl_ast_build_free)
UniquePtr(AstExpr, isl_ast_expr,  isl_ast_expr_free)

S.copy() returns a raw pointer refering to a copy of S.

Why isn't this returning a managed pointer?

Thanks for your interest.

The use of copy() is to interact with isl's C interface (and other code using raw pointers to isl objects). These functions take parameters annotated either with __isl_keep or __isl_take. __isl_take parameters assume to receive ownership of the isl object and will free it before they return. If the caller still needs the isl object it has to create a copy to it. This is what copy() does.

That is all really weird and unexpected from a C++ API. I would expect a C++ API to *hide* all this.

There is no use for copy() when using the C++ binding only. Simple assignment Dst = Src already copy the object implicitly (value-like semantics).

Oh, that's not really a smart pointer then, that's kind of answering my question about unique_ptr above.

Btw, 'copy' does means increasing the reference counter, not a literal copy. The isl library considers this an implementation detail.

Mmmm, now I'm not totally sure what Dst = Src do :)

lib/External/isl/include/isl-noexceptions.h
140

Does not seem formatted.

161

Almost all these are one liner, why don't you just define them here in the class?

Also, do they all need to be inline? It'll incurs compile time overhead without good reason otherwise.

What big difference with std::unique_ptr (and a custom deleter) justify to rewrite so much code for your smart pointer?

In addition to using isl's intrusive reference counting system (that is, these are more like shared_ptr than unique_ptr), we are generation methods that apply operations on them. For instance

Result = set.apply(map);

The use of copy() is to interact with isl's C interface (and other code using raw pointers to isl objects). These functions take parameters annotated either with __isl_keep or __isl_take. __isl_take parameters assume to receive ownership of the isl object and will free it before they return. If the caller still needs the isl object it has to create a copy to it. This is what copy() does.

That is all really weird and unexpected from a C++ API. I would expect a C++ API to *hide* all this.

We cannot do without interacting with C code, yet alone because the C++ bindings do not implement (yet) all functionality possible with isl so we sill need to call the C API.

Btw, 'copy' does means increasing the reference counter, not a literal copy. The isl library considers this an implementation detail.

Mmmm, now I'm not totally sure what Dst = Src do :)

Something like this:

void Map::operator=(const Map &M) {
  isl_map_free(this->Ptr);
  this->Ptr = isl_map_copy(M.Ptr);
}

You talked about "value semantic" earlier, but we have smart pointer, and you mentioned reference counting. Overall I have no idea what's going on if I were to use these class.
I assume that's fine, as long as people that are familiar with ISL are happy with it, but I'd hardly consider this a "C++ API" at this point, seeing how much it seems very intrinsically tied to "the ISL way" of handling its internal and does not seem to be expressed in a way that would match common C++ ownership management idioms.

You talked about "value semantic" earlier, but we have smart pointer, and you mentioned reference counting.

I don't see a contradiction here. Value semantics can be implemented using copy-on-write.

Overall I have no idea what's going on if I were to use these class.
I assume that's fine, as long as people that are familiar with ISL are happy with it, but I'd hardly consider this a "C++ API" at this point, seeing how much it seems very intrinsically tied to "the ISL way" of handling its internal and does not seem to be expressed in a way that would match common C++ ownership management idioms.

Being "tied to the ISL way" is to some degree required to become official isl C++ bindings.

Can you elaborate on why a C++ API should match a C++ ownership management idiom? Is glibmm a C++ API? How would you design a C++ wrapper given the isl C-API design? We discussed the API's design extensively here.

You talked about "value semantic" earlier, but we have smart pointer, and you mentioned reference counting.

I don't see a contradiction here. Value semantics can be implemented using copy-on-write.

I see a contradiction between "value semantic" and "shared_ptr" semantic. (you also wrote earlier " that is, these are more like shared_ptr than unique_ptr").

I also don't think I've read "smart pointers" associated with "value semantic" before (in the context of C++). I.e. I wouldn't present my class as a "smart pointer" if it has "value semantic".

Overall I have no idea what's going on if I were to use these class.
I assume that's fine, as long as people that are familiar with ISL are happy with it, but I'd hardly consider this a "C++ API" at this point, seeing how much it seems very intrinsically tied to "the ISL way" of handling its internal and does not seem to be expressed in a way that would match common C++ ownership management idioms.

Being "tied to the ISL way" is to some degree required to become official isl C++ bindings.

Sure, I'm approaching it from a "C++ should be first class citizen" point of view: i.e. I have little to no interest in "the ISL way of doing things around a C API", which I totally understand can't be compatible with designing "official bindings".

Can you elaborate on why a C++ API should match a C++ ownership management idiom?

Because otherwise it does not fit the idiom that a C++ programmer is expecting to see.
For example, I don't believe it would be acceptable for any LLVM component to do this without a very good motivation.

Keep in mind that I'm not an ISL user, I only played with it a little bit 5 years ago. I also remove "C" from my resume for good reasons :)

Hi Mehdi,

thanks a lot for looking into these bindings.

I probably should have explained the overall effort better in the commit message. Reading it again I understand that for whoever was not involved in recent discussions regarding isl and C++ bindings has a hard time getting the context here. Let me try to provide some context:

isl is a math library that provides functionality to perform mathematical operations on integer sets and some other objects (I assume you know this). It is implemented as C code with explicitly managed reference counting. Hence a simple piece of isl code (no useful semantics) would look as follows:

__isl_give isl_set *do_some_operations(__isl_take isl_set *s1, __isl_take *isl_set *s2, __isl_keep isl_set *s3, __isl_take isl_set *s4) {
  isl_set *s5 = isl_set_intersect(isl_set_copy(s1), s2);
  s5 = isl_set_subtract(s5, isl_set_union(isl_set_copy(s3), s4);
  if (isl_set_is_empty(s5)) {
    isl_set_free(s1);
    isl_set_free(s5);
    return isl_set_copy(s3);
  } else {
    isl_set_free(s5);
    return s1;
  }
}

You can see that we have explicit memory management annotations called isl_give (returns a reference), isl_take (takes and later frees a reference), isl_keep (takes and preserves a reference). Also, each function call is rather lengthy, as the function names contain the names of the affected types (no overloading).

Now, let's ignore this specific patch for the moment. The very same code as above can be written in C++ a lot more concise using class methods, operator overloading, and automatic memory management.

isl::Set do_some_operations(isl::Set s1, isl::Set s2, isl::Set s3, isl::Set s4) {
  isl::Set s5 = s1.intersect(s2);
  s5 -= s3 + s4;

  if (s5.isEmpty())
    return s3;
  else
    return s1
}

As hopefully becomes visible, the C++ code now looks almost like mathematical expressions and all pointer handling is nicely hidden from the day-to-day user. I believe this code is now close to optimal in terms of readability.

This is what we are aiming for with the isl C++ bindings. Does this make sense to you?

Besides this goal of increased readability we also want to introduce these new isl bindings in a -- LLVM typical -- gradual way. Hence, we often will have functionality in isl or in Polly that still uses the old C interface. To interact with such functionality we need access to the raw pointers. Assume that for unknown reason the functions isl_set_union and isl_set_is_empty are not yet available as part of the C++ interface (maybe these are very complex functions we implemented in Polly itself and did not yet manage to port over). Then we want to interact with these functions, but already would like to move other parts of Polly over to the C++ bindings. Hence, we get a mix of C and C++ code, that could look as follows:

isl::Set do_some_operations(isl::Set s1, isl::Set s2, isl::Set s3, isl::Set s4) {
  isl::Set s5 = s1.intersect(s2);

  s5 -= isl::manage(isl_set_union(s3.copy(); s4.release()));

  if (isl_set_is_empty(s5.get()))
    return s3;
  else
    return s1
}

Here we use isl::manage to convert a raw isl C pointer back to the C++ object, and we use isl::Set::copy() and isl::Set::release() to obtain raw pointers from s3 and s4, that represent a set identical to s3 and s4, which can be passed and later be consumed by isl_set_union. For object s3 we need to use copy() to obtain a reference-counted copy of the s3 pointer, as the s3 object may possibly be used later. For s4 we can use release() which extracts the pointer that backs s4 and invalidates s4. This is possible as s4 is not used later on. Similarly, we use s5.get() to obtain a raw pointer that can be passed to isl_set_is_empty but that does not have to be consumed by isl_set_is_empty.

For reference:

__isl_give *isl_set_union(__isl_take isl_set *s1, __isl_take *s2);
int isl_set_is_empty(__isl_keep isl_set *s);

Now back to this patch. In some way we start backwards. The first step we take is to assume no function is managed through the C++ interface. Hence, we only introduce almost empty C++ classes that can take ownership of a pointer, free the underlying object at the end of its lifetime and provide access to different kind of raw pointers.

In subsequent patches, we introduce a function interface (https://github.com/tobig/polly/commit/c05fa3e6f4106b0801979038142cb3657bb64dd9), and translate different parts of Polly to this C++ method interface (https://github.com/tobig/polly/commit/4b3d621f25c91e3cb4c3ce57ef24a630df355786, https://github.com/tobig/polly/commit/4e4361805eae7056e6feadce1af44952210b070d, https://github.com/tobig/polly/commit/d9cc363370202a1bdb867f4e928ab11532edc686). I am still in the process of polishing these patches, but hope to put them soon on phabricator to provide a more complete picture. However, even the draft commit messages and the associated changes should already today give a pretty good picture on the kind of simplifications these new C++ bindings will enable.

A lot more details on the motivations of parts of these bindings can be found in the in-process commit messages that implement the binding generator: https://github.com/tobig/isl/commits/cpp-generator
Feel free to skim through these commits or just ask for specific points here on the review thread.

I believe these bindings are meanwhile pretty mature and a great step forward. I am pretty confident what we came up with is rather solid, as we worked with many people who have experience with developing isl C++ before.
Still, it is always very good to get unbiased feedback from somebody who did not take part in the development. Hence, I am very glad to see you in this review thread. Feel invited to raise any questions, comments, concerns you have in this and the upcoming reviews.

Best,
Tobias

@mehdi_amini Alexandre yesterday pointed me to the canonical document of what we are doing: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3339.pdf These are indeed "smart pointer[s] that applies value semantics to its pointee". I will update the commit message to make this very clear. Thanks for pointing out that the pure use of smart pointers just causes confusion.

grosser updated this revision to Diff 90609.Mar 5 2017, 8:13 AM

Update to lastest isl C++ bindings

grosser updated this revision to Diff 91269.Mar 9 2017, 10:46 PM

Update header guard to ISL_NOEXCEPTIONS and update to latest Polly

grosser retitled this revision from [Polly] Introduce isl C++ bindings - Part 1 (smart pointer interface) to Introduce isl C++ bindings, Part 1: value_ptr style interface.Mar 9 2017, 10:47 PM
grosser edited the summary of this revision. (Show Details)
grosser edited the summary of this revision. (Show Details)

Dear all,

I rebased to the latest Polly commit, updated the commit message to explain the missing points @mehdi_amini raised (value_ptr interface and why we generate our own classes). I also changed the header guard to ISL_NOEXCEPTIONS as suggeted by @Meinersbur.

I believe now all concerns are addressed.

grosser updated this revision to Diff 91272.Mar 10 2017, 12:03 AM
grosser edited the summary of this revision. (Show Details)
  • Rename isl::obj::get_str to isl::obj::to_str
  • Add "/* implicit */" markers where appropriate
grosser updated this revision to Diff 91285.Mar 10 2017, 1:21 AM

Make clang-format clean (minor whitespace fixes)

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