Page MenuHomePhabricator

[isl] Typesafe user pointers
Needs ReviewPublic

Authored by philip.pfaffe on Jul 20 2018, 10:44 AM.

Details

Summary

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.

Event Timeline

philip.pfaffe created this revision.Jul 20 2018, 10:44 AM

Missed a couple of c-api uses

Looks good, I haven't looked too much into how Any works, which I will.

lib/Analysis/ScopInfo.cpp
1212

I believe this assert can be removed?

2083–2084

const auto *Param?

lib/CodeGen/IslNodeBuilder.cpp
295

const auto *?

1051

const auto *?

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

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
293

This assert can be removed?

philip.pfaffe marked 6 inline comments as done.

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.

bollu added inline comments.Jul 20 2018, 1:06 PM
lib/Analysis/ScopInfo.cpp
1212

Ditto here. The assert was not removed. We don't need it anymore since get_user should check this?

lib/CodeGen/IslAst.cpp
293

I am confused, this assert still exists?

Handle nullptr users correctly

Address @bollu's comments

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.