cpp: export isl_id

@nicolasvasilache

Exporting isl_id is non-trivial as it takes a void* user pointer. I wonder if there are better ways to export this function than just exposing the userpointer?

@nicolasvasilache : Your private branch has a managed_isl branch and I can see some templated version of isl_id which you developed. Any idea who wrote the code, if this is automated and if this is something we could upstream.

This currently blocks me from exposing most of the list stuff.

@TobiG

@TobiG I wrote it.

The problem with the userpointer is that isl may need to destroy it internally after the C++ id object goes out of scope. For example, an isl::id is created manually and used as a tuple id in a space. The space is used to construct the domain union_set, for which we compute the schedule. The C isl_id object survives throughout the process while the C++ isl::id may be long gone. So we want to store the user data in the C isl_id. The rest of the wrapper is necessary for two reasons:

allow capturing lambdas to be passed as deleters, which are expected to be pointers to C functions;
provide some amount of type-safety, in particular make sure the user-provided deleter takes an instance of the class stored in user pointer rather than void*, so we can safely pass a wrapped destructor.

My understanding that isl_id is essentially shared_ptr over the pair userpointer+name. It provides reference counting and supports a custom deleter exactly like shared_ptr, so I would expect it to behave similarly.

We considered an alternative encoding where user data is stored in a hashmap (isl::id)->(std::string, isl::data_wrapper_class) in the context to which the id belongs, but it (1) introduces more global-like state than I would like and (2) maintains user data alive longer then potentially necessary. It could be possible to put a dummy user object to the C isl_id and trigger the deletion of the user object from the C++ hash map, but this boils down to a more involved version of the implemented solution.

The managed_isl is experimental and was not even tested with out stuff. I expect to make a local PR soon, and if it goes in, replicate it here.

Interesting, I came up with the following "approach" for type safety:

template <typename T>                                                            
class managed_id : public isl::id {                                              
        const char *type_address;                                                
        static const char type_token;                                            
        T payload;                                                               
                                                                                 
public:                                                                          
        managed_id(isl::ctx ctx, std::string name, T payload)                    
                : isl::id(ctx, name, this), payload(payload) {                   
                type_address = &type_token;                                      
        }                                                                        
                                                                                 
        T get_payload() {                                                        
                managed_id *user = (managed_id *) get_user();                    
                ISLPP_ASSERT(user->type_address == type_address,                 
                             "Unexpected type identifier");                      
                return user->payload;                                            
        }                                                                        
};                                                                               
                                                                                 
template<typename T>                                                             
const char managed_id<T>::type_token = '\0';

which is used as follows:

        isl::ctx ctx = isl_ctx_alloc();                                          
        managed_id<int> ID(ctx, "aa", S);                                        
                                                                                 
        std::string S2 = "hallo";                                                
        managed_id<std::string> ID2(ctx, "aa", S2);                              
                                                                                 
        printf("%d\n", ID.get_payload());                                        
        printf("%s\n", ID2.get_payload().c_str());

The idea is that we can use the address of a type specific static global to uniquely identify the type of the content and verify if the isl::id used to read the type is indeed of the same type as the isl::id used to store the type.

The above snippets are still broken as the isl pointer still references the isl id object, instead of an object that can be freed when the isl_id is destroyed. But the idea itself might be useful.

@ftynse

@ftynse : Yes, it would be good to have a pull request to comment on.

@TobiG

@TobiG cool trick. I think it can be combined rather easily with what we have.

I did not want to have a class template for the id type itself because it could prevent me from having an (stl-style) list of ids, which expects instances of the same class. Particularly, the size of managed_id depends on the size of its template argument. List of pointers + inheritance would solve the problem, but it seemed inconsistent with the no-pointer-and-use-copy-everywhere semantics of the other parts. Also, isl_id being essentially a shared_ptr, taking a pointer to a shared pointer looks weird.

We can hide this under the template function get<T>, and additionally introduce try_get<T> / get_or_null<T> and isa<T> for probing.

Sounds great. Yes, I think the two ideas are quite complementary and can easily be combined. I like your non-templated class as this also avoids the need to return an isl_id in get_id() functions and later cast it to something else.

How did you generate this class. Do you have patches to the isl C++ generator?

So far it is hand written in isl.h.top. Not sure whether we want to patch the generator instead. My understanding is that we don't necessarily want the generator to have a large block of code it just prints instead of having the code directly.

I was mostly interested how you did it. I think starting off from a class that is defined in isl.h.top is a good idea. We can then still see if we want to automate some of the generation. It might indeed not be needed.

More discussion:
calls to isl_id_alloc with the same arguments give you same (pointer-comparable) ids. If we want to preserve the same behavior in C++ with an additional wrapper, we will have to maintain a map "user pointer -> wrapped user pointer" on the C++ side ourselves. This is tricky in a header-only interface...

I don't understand. Do you want that the pointer addresses of two isl::ids compare equal if their underlying isl_ids compare equal. Or do you want them to compare equal for std::set and Co. I think in this case, we should overload std::less or operator<, no?

isl_ctx *ctx = isl_ctx_alloc();
int i = 42;
isl_id *id1 = isl_id_alloc(ctx, "name", &i);
isl_id *id2 = isl_id_alloc(ctx, "name", &i);
assert(id1 == id2); // this holds!

it would make sense to have the same behavior in C++

isl::ctx ctx(isl_ctx_alloc());
int i = 42;
isl::id id1(ctx, "name", &i);
isl::id id2(ctx, "name", &i);
assert(id1 == id2); // expect to hold
assert(id1.get() == id2.get()); // isl should also know that these ids are the same.

Because we do not put a raw pointer to a C++ object, but a wrapper we constructed internally, isl will be unable to match user pointers. We have to keep track of the internally-constructed wrapper objects.

I see. In this case, I don't think we can reasonably piggy-pack anything to the user pointer. I also don't believe that keeping a separate map in the C++ interface is the right solution. Instead, I would likely add an additional field to isl's isl_id struct and add an interface that allows the C++ bindings to officially store the data it needs.

I don't see how an extra pointer would help. My user pointer-packing magic is intended to accept std::function as a deleter instead of a C function pointer. We cannot decay an std::function to a function pointer, so we store it instead and provide a deleter function that recovers the std::function and calls it.

An alternative would be keep C function pointer as a deleter in C++ interface. At this point, the id can be treated just like any other class.

You could do your very same packing and pass the pointer you pass today in the extra pointer and the original pointer as key in today's pointer field, could you?

Regarding the alternative. I think it might make sense to start with the most simple solution that matches the C interface as closely as possible. Could we just export isl_id with __isl_export and then add the missing functionality to the generator to generate code for it as it is (or write the code that would be generated by hand). In the end we want the C and C++ interface to be reasonably similar.

Well, the easiest solution would be to force the deleter in the C++ interface to be a C function pointer.

Otherwise, having two user pointers in the C isl_id seems weird...
However, we can try to get around that by arguing that usr1 is included in the identity of isl_id and usr2 is not. So isl_ids with equal usr2 pointers are not guaranteed to be equal.

But I still don't see how that would help. It would either break pointer-comparability of ids in C, or require the second pointers to be the same to get you the same id object, which brings us back to the original problem: maintain a mapping between actual user pointers aka usr1 and extra data created for that pointer aka usr2 in order to create identical pointers.

@TobiG

@TobiG @nicolasvasilache
So here are my two propositions https://github.com/nicolasvasilache/isl/pull/17 https://github.com/nicolasvasilache/isl/pull/18.

We may want to keep https://github.com/nicolasvasilache/isl/pull/18 and propose https://github.com/nicolasvasilache/isl/pull/17 in a documentation as a way for the user of islpp library to have arbitrarily-typed deleters and preserve the isl_id pointer-comparability.

As discussed face-to-face, I am largely in favor of the lightweight version. As long as we let lifetime management happen in C I do not see why we'd want to allow the deleter to capture.

…

On Fri, Aug 4, 2017 at 12:37 PM, ftynse ***@***.***> wrote: @TobiG <https://github.com/tobig> @nicolasvasilache <https://github.com/nicolasvasilache> So here are my two propositions nicolasvasilache/isl#17 <https://github.com/nicolasvasilache/isl/pull/17> nicolasvasilache/isl#18 <https://github.com/nicolasvasilache/isl/pull/18>. We may want to keep nicolasvasilache/isl#18 <https://github.com/nicolasvasilache/isl/pull/18> and propose nicolasvasilache/isl#17 <https://github.com/nicolasvasilache/isl/pull/17> in a documentation as a way for the user of islpp library to have arbitrarily-typed deleters and preserve the isl_id pointer-comparability. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJradGq-OD_rOJrSjrOuj5vbeh-ACiY8ks5sUvRggaJpZM4OqEOI> .

@ftynse

@ftynse , I also believe the lightweight version is preferable for now. Would you be interested in preparing a patch that is upstreamable? It seems the code is very close already.

I would suggest to auto-generate the code, as this will make the patch a lot smaller and would raise less concerns during a patch review.

You can likely just:

export isl_id
teach the types about void_ptr
Fix the printing of the callback setter

I would probably start without additional comparison operators or constructors and would for now also not move the deleter into the constructor. I would possibly even not template the code for now.

While this removes some of the features we (mostly you) came up with, it is likely a very good start and very uncontroversial.

For #11 having isl_id exported would be useful.

Navigation Menu

Repository navigation

cpp: export isl_id #8

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

cpp: export isl_id #8

Description

Activity

ftynse commented on Aug 1, 2017

ftynse commented on Aug 1, 2017

tobiasgrosser commented on Aug 1, 2017

tobiasgrosser commented on Aug 1, 2017

ftynse commented on Aug 1, 2017

tobiasgrosser commented on Aug 1, 2017

ftynse commented on Aug 1, 2017

tobiasgrosser commented on Aug 1, 2017

ftynse commented on Aug 3, 2017

tobiasgrosser commented on Aug 3, 2017

ftynse commented on Aug 3, 2017

tobiasgrosser commented on Aug 3, 2017

ftynse commented on Aug 3, 2017

tobiasgrosser commented on Aug 4, 2017

tobiasgrosser commented on Aug 4, 2017

ftynse commented on Aug 4, 2017

ftynse commented on Aug 4, 2017

ftynse commented on Aug 4, 2017

ftynse commented on Aug 4, 2017

nicolasvasilache commented on Aug 4, 2017

tobiasgrosser commented on Aug 4, 2017

tobiasgrosser commented on Aug 18, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions