This is an archive of the discontinued LLVM Phabricator instance.

Demo (Proof of Concept) of isl++
Needs ReviewPublic

Authored by simbuerg on Nov 12 2014, 11:26 AM.

Details

Reviewers
zinob
Summary

This shows a concept of isl++ inside polly's dependency analysis. This shows how a transition from C-isl to isl++ might look.
In order to demonstrate a few features of it, some parts look more horrible than they have to in the end.

Diff Detail

Event Timeline

simbuerg updated this revision to Diff 16108.Nov 12 2014, 11:26 AM
simbuerg retitled this revision from to Demo (Proof of Concept) of isl++.
simbuerg updated this object.
simbuerg edited the test plan for this revision. (Show Details)
jdoerfert edited edge metadata.Nov 12 2014, 12:10 PM

Some comments from my side. If you guys like I could push a version with mutable objects to make the difference clearer.

include/polly/Dependences.h
132

Why do we need these unique pointer (or pointers) in general?

150

We do use references here so why the pointers above?

159

I would translate __isl_take to a XValue:

isl::Map &&Deps

to make it clear that we loose ownership here.

lib/Analysis/Dependences.cpp
90

I still don't get the Wrap thing. Why isn't there a constructor for isl::Set that takes an isl_set * ?

Furthermore, the isl namespace is open here, is it not?

92

I would like it to be:

AccDom.intersectDomain(std::move(Domcp));
107

see above (mutable objects)

109

see above (::Wrap)

116

isl::DTOut doesn't seem to be a good name for isl_dim_out, maybe isl::kDimOut as it is a constant (or enum).
Same for isl::DTin.

117

This doesn't look right, Scatter is moved, thus it is not a valid object anymore, however you still use it in the next iteration. Either the move is wrong (and addMap should get a const isl::Map &) or the Scatter object needs to be copied.

121

Now I'm confused, above you move the Set into addMap here you just pass it. What's the type of addMap ?? Does isl::Map have a default copy constructor (I hope not).

177

I don't like the idea of mixing the nice memory managment with some pointer magic we need to keep track of?
What's the reason we cannot use objects/references here?

189

We are still not using lambdas in Polly and I'm still not sure if we should.

222

Why is the member function called union_ and not union ?

232

What is FIsl ?

240

Now we mix object references, object pointers and null pointers, that looks weird.

242

Sometimes you use the implicit conversion (here Map -> UnionMap) and sometimes you don't (somewhere above)

342

Here is an explicit conversion again.

388

I don't get this statement. Is there some static magic happening in isl::Context ?
If so I don't like it. Why should we make a difference (except copy and free calls internally) between isl::Context and other isl:: objects?

391

leftover

408

I do not think this is nicer than what we had before :(

lib/CodeGen/IslAst.cpp
37

leftover

simbuerg added inline comments.Nov 12 2014, 3:58 PM
include/polly/Dependences.h
132

We don't. It was just an easy way to map the previous code to an isl++ version, without managing the cleanup myself.

159

true.

lib/Analysis/Dependences.cpp
90
  1. Yes, that will change. Didn't get to it yet. We discussed it in the other mail, there will be a constructor.
  1. Yep. ;-)
92

No, because I do not want to break with isl's interface.

107

I don't think the copying/freeing is bad enough to break with isl's interface.

116

It is auto-generated, so any suggestion will be fine I think ;-).
It's just enum isl_(d)im_(t)ype -> DT<enumval>

117

Uh thanks, didn't notice that yet.

121

I didn't place the std::move calls consistently yet. Usually we take const-refs and our copy constructor is, in fact, making a copy of the wrapped isl object.

177

see explanation above when i declare the unique_ptr. The alternative would require us to either:
a) Manage a raw pointer ourself, or
b) Access the wrapped pointer from outside to check if we
stored null in there (hasValidDependencies()).

I do not want to use a) (that's what a smart pointer is for) and
I do not want to make use of b), except for _leaving_ isl++, therefore, accessing the wrapped pointer releases ownership from the isl++ object.
We could do b) here, maybe.

189

What is scary about lambda's? In here it's a demo that we can use them as callbacks just fine within isl (with a little help from templates).

222

union is a keyword in c/c++

232

enum isl_format { ISL, ... } -> isl::FIsl. auto-generated from the exported isl types

240

Well, computeFlow wants:
optional(!) output parameters. So, we have to be able to pass null into it as well as supply some storage in case we're interested in the result. I'm not 100% happy with the signature here, but it works.
I do not want to manage storage myself with the use of **.
Feel free to suggest a different type signature here.

242

That is a typo, sorry for the confusion.

342

that is a function defined by isl: Isl_union_set_from_set(..). In isl++ they're implemented as static functions ("named constructors").

388

No there shouldn't. I agree. At the moment it is a corner that I avoided. isl++ manages just one global context so far and here we feed it the context we want, once. The rest happens automatically (we derive the context from the parameters).

408

It is nicer than a pointer that you have to free yourself. Just doesn't look nicer.

lib/Analysis/ScopInfo.cpp
1782

Don't get confused here, I'm still debugging, this is not intended.

lib/CodeGen/IslAst.cpp
37

Should be a separate patch, yes. IslAst.cpp uses printer methods without including it.

grosser added inline comments.Nov 12 2014, 11:24 PM
lib/Analysis/Dependences.cpp
116

What about isl:dim_type::enumval? I do not see a need to abbreviate this.

177

I think this is actually a nice example that smart pointers now work nicely with isl objects, as the isl_*_free functions will be automatically called on deallocation.

I dislike b) as it complicates the semantics of isl objects. Now a union_map can not only be empty, but it can also be the NULL pointer. If we want/need a maybe type, we really should use smart pointers.

I agree that this particular piece of code looks a little ugly. We could possibly try to make it nicer by removing the maybe character of this type all together. However, I regard this as an issue orthogonal to isl++. (Johannes, maybe this particular piece of code can be removed with the reductions as MayWrites suggestion Sven sent out a couple of days ago)

189

I personally think it is nice to support lambdas. To which extend we want to use them in Polly can possibly be discussed on a case by case basis. I personally like them, but would probably only use them for rather small and mostly trivial functions.

232

what about isl::format::iSL

388

I think that is something which needs to be clarified. I think it is convenient to have a default context which at best could be set by us to the one created in Polly, but we need to make sure this has sane semantics and that it is still possible to pass in arbitrary context objects to isl if needed.

408

I think this is the correct translation for the code as it is here, but it is also a sign that the code could (in an independent patch) be written nicer here.

We could e.g. move the ScatteringSpace computation out of the loop and make it something like.

IslSet::fromUnionSet(Scattering.range()).space()

This opens up the question of which signature has fromUnionSet, as it can return either a nullptr or an IslSet.

jdoerfert resigned from this revision.Sep 27 2015, 6:04 PM
jdoerfert removed a reviewer: jdoerfert.

This revision seems to be dead.

grosser resigned from this revision.Feb 25 2016, 12:37 PM
grosser removed a reviewer: grosser.

This revision is outdated. We should open a new review after Andreas/Armin submitted the updated isl bindings.

sebpop resigned from this revision.Sep 19 2016, 1:19 PM
sebpop removed a reviewer: sebpop.