With the new llvm::any, we can finally make the user pointers in
isl_id's typesafe! The change makes isl-noexceptions.h depend on llvm, I went
this way for a nicer API.
Details
- Reviewers
grosser Meinersbur bollu
Diff Detail
- Repository
- rPLO Polly
- Build Status
Buildable 20600 Build 20600: arc lint + arc unit
Event Timeline
Looks good, I haven't looked too much into how Any works, which I will.
lib/Analysis/ScopInfo.cpp | ||
---|---|---|
1211–1212 | I believe this assert can be removed? | |
2083–2085 | const auto *Param? | |
lib/CodeGen/IslNodeBuilder.cpp | ||
295 | const auto *? | |
1051 | const auto *? | |
lib/External/isl/include/isl/isl-noexceptions.h | ||
6998 | I don't know if I like an assert here. I wish there was someway to enforce this? Is the cost way to high to actually have an if + report_fatal_error? |
Looks good, I haven't looked too much into how Any works, which I will.
lib/CodeGen/IslAst.cpp | ||
---|---|---|
291–293 | This assert can be removed? |
Fail harder if the wrong type is given.
I don't like spelling out constness on auto when the full type is on the right
hand side.
Fixery to appease the regression tests:
- Back out of cool new api in PPCGCodeGeneration.
- Don't delete Any through void*
- Fix a get_user<const>
Hi Philip.
we discussed this in private already. I think in general this is a good idea, but it would be nice to upstream this to the isl bindings generator:
I pushed a first patch that implements isl:🆔:alloc using std::any:
https://github.com/tobig/isl/commit/8ee4b80e920897b61daba0c290bbd142fa2a9d8e
It's a proof-of-concept, but shows how to add extensions to the C++ bindings generator.
Not super-cleaned up, but might be a start in case you want to hack on it.
One problem is that isl idscurrently do not compare equal if std::any is used:
$cat test.cpp
#include "isl/cpp.h"
#include "isl/id.h"
#include "assert.h"
int main() {
isl::ctx ctx = isl_ctx_alloc();
{
int I = 12;
isl::id id(ctx, "test", I);
I = *id.get_user<int>();
isl::id id_1(ctx, "test", &I);
isl::id id_2(ctx, "test", &I);
isl_id_dump(id_1.get());
isl_id_dump(id_2.get());
assert(id_1.get() == id_2.get());
printf("%d\n", I);
}
isl_ctx_free(ctx.get());
return 0;
$./a.out
test@0x56188ecb4220
test@0x56188ecb42a0
a.out: test.cpp:20: int main(): Assertion `id_1.get() == id_2.get()' failed.
Aborted (core dumped)
The C interface ensures that two id's that are generated with the same values become the same object.
The following discussion is also relevant:
https://github.com/PollyLabs/isl/issues/8
Best,
Tobias
As we discussed offline, std::any doesn't really help polly, because it's a c++17 feature. Instead, what about making the generator itself extensible? Allow users to hook into the generation process and modify whatever happens. Then we can just emit llvm::Any privately. Additionally, this can settle the exceptions/no-exceptions debate.
Regarding the reference identity of IDs: Do we care? Is it important to reflect this behavior in the c++ API? As in, do we ever rely on this?
I used the std::any variant as an easy way to showcase how the bindings generator could be modified to do what we want (and also to show how an ideal interface could look like). I feel we should bring this up on the isl mailing list first to see what would be a good way to add this feature. We potentially could replace std::any with a simpler C++11 compatible solution:
http://brotherst-code.blogspot.com/2015/10/how-to-get-typeindex-without-rtti.all
Not sure if this is worth making the generator "pluggable". I feel this feature might trigger broader interest.
The link is 404, but I assume it boils down to the same thing as either std::any or llvm::Any. It's not hard to reroll the implementation in isl, but then we have three.
I think making the generator customizable is orthogonal to adding std::any. But it opens to door to adding std::any into isl proper, and _replacing_ it with llvm::Any on the polly end. I feel this makes for a cleaner integration between the two.
I believe this assert can be removed?