pollylabs / isl Goto Github PK

Essentially, isl::ctx is used as a "global" state for thread-safety reasons. We may want to declare one thread_local isl::ctx and use it implicitly in all functions that require a context.

It would be nice to automatically format isl patches. The tool clang-format was developed precisely for this: https://clang.llvm.org/docs/ClangFormat.html

Unfortunately, it does not yet seem to support everything needed for isl:

https://bugs.llvm.org/show_bug.cgi?id=33835

Current callback support assumes callbacks return isl_stat, which is not remotely the case everywhere.
In particular, schedule tree traversals and ast_build may return either isl_bool or the same type as the first argument of the callback.

Was there any attempt to export container types like isl_*_list and isl_*_to_* ?

I can see either some conversion to std::vector and std::unordered_map, or exposing STL-style iterators from otherwise opaque classes.

It can fit a custom stream object, similarly to std::stringstream, which has state and can accept any relevant objects using operator <<. This way, it will be possible to configure the output formats of the isl_printer. The different between printer_to_file and printer_to_str can be resolved, e.g., with inheritance. It would be nice if printer_to_str behaved similarly to std::stringstream. Finally, callbacks accepting/returning a printer object can accept/return a stream object instead, which is compatible with C++ notion of a stream.

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.

At times, 'isl_tab_basic_set_non_trivial_lexmin' does not return the lexicographically smallest solution. It appears that the order of rows in the trivial matrix within the trivial region may influence this outcome.

I found myself writing
isl::manage(isl_whatever_copy(o)); // manage releases!
multiple times.

Can we generate an overloaded function

isl::whatever managed_copy(__isl_keep isl_whatever *w) {
  return isl::manage(isl_whatever_copy(w));
}

as a shortcut for these cases?

Currently, only isl::stat is supported. We can follow the same strategy as isl::stat and all enums to isl.h.top and generate appropriate static_casts while generating methods. We need to decide on names of enums as the simple rule of removing type name from enums will result in C++ default keywords for some like isl_ast_node_for, isl_ast_node_if etc..

What would be the best way to add support for exceptions in the interface? Generate different bindings with a flag to generator?

Ideally, I would like to have meaningful messages inside exceptions. For example, include messages passed to isl_die. The main problem is that internal memory management relies on the errors being propagated until the user call through isl_stat/isl_bool or null pointers to do some cleenups. So we cannot just throw on isl_die. We could try to copy the message into the isl_ctx and then query it, similarly to errno in POSIX. This would complicate the wrapper functions and make them do the following:

• find the ctx of their arguments;
• do the call and check if it did not return error/null (except if marked as __isl_null);
• if did return error, query the context and throw
• stack unwinding should take care of clearing the relevant C++ wrappers.
  This requires that all functions that may fail could obtain the context from their arguments. Is this the case?

Any other thoughts?

In particular, std::set and std::map expect objects to be less-than-comparable and std::unordered_set and std::unordered_map to have a specialization of std::hash.

C++11 stuff could go in a separate file that is conditionally included based on the C++ version.

Memory management and lifetime of isl_ctx objects seems quite different from the ones that islpp automatically extracts. Let's not pretend the API should be the same (i.e. get, release etc), it feels more confusing than anything else.
From skimming through uses of isl::ctx it seems it is alway used with .release().
How about dropping all these raw pointer accessors and just implicitly convert to isl_ctx* ?

Currently, the exported function is made static if its first argument is of a different type than its expected surrounding class (determined from the function name prefix).

There are "constructing" functions which make more sense as static rather than as instance functions, but with current generator it seems impossible to make them static. In particular, space construction:
isl_space_map_from_domain_and_range(isl_space *, isl_space *) makes more sense if translated as
isl::space::map_from_domain_and_range(domain, range) and not as domain.map_from_domain_and_range(range). Same goes for, e.g., isl_space_set_from_params.

I would suggest an extra tag __isl_export_ensure_static that forces the generator to produce a static function even if the function would be instance function otherwise. The danger here is to avoid a mixup with all the implicitly static functions with only __isl_export.