This is an archive of the discontinued LLVM Phabricator instance.

Port ScopInfo to the isl cpp bindings
ClosedPublic

Authored by philip.pfaffe on Nov 13 2017, 12:09 PM.

Details

Summary

Most changes are mechanical, but in two places I changed the program semantics
by fixing bugs, which I'd like a review on.

The first is in Scop::buildSchedule(Region *, LoopStackTy &, LoopInfo &).
Before, we took a reference to LoopStack.back() which is a use after free,
since back is popped off further below. This didn't crash before by pure
chance, since LoopStack is actually a vector, and the memory isn't freed upon
pop. I turned this into an iterator-based algorithm.

The second is Scop::hasFeasibleRuntimeContext(), where I'm now explicitely
handling the error-case. Before, when the call to
addNonEmptyDomainConstraints() returned a null set, this (probably)
accidentally worked because isl_bool_error converts to true. I'm checking for
nullptr now.

Diff Detail

Repository
rL LLVM

Event Timeline

philip.pfaffe created this revision.Nov 13 2017, 12:09 PM

Split the fix for the use-after-free out of this into a seperate patch.

Hence this diff now depends on D39979.

Meinersbur accepted this revision.Nov 14 2017, 7:27 AM

LGTM, only nitpicks. Thanks for your work.

include/polly/ScheduleOptimizer.h
74–75 ↗(On Diff #122724)

Is this change related?

I think it is a good idea to include ScheduleTreeOptimizer in the Polly namespace, can you commit it separately?

include/polly/ScopInfo.h
1696–1702 ↗(On Diff #122724)

Why the move?

[Note] Seems to be wrong in the original comment:

By doing this, we guarantee that the context is deleted when we delete the last object that creates isl objects with the context.

Isn't the point that the isl_ctx is deleted /only after/ all its isl objects are released? I mean, the isl objects don't participate in this shared_ptr and therefore cannot trigger the release of the isl_ctx.

include/polly/Support/GICHelper.h
180–215 ↗(On Diff #122724)

For consistency with the other stringFromIslObj functions, shouldn't these be defined in the .cpp file?

The isl objects have to_str() methods, and we have overloaded the << operators in ISLOStream.h; consider using them instead.

lib/Analysis/ScopInfo.cpp
2212 ↗(On Diff #122724)

[Style] No almost-always-auto.

2558 ↗(On Diff #122724)

Consider isl::set::universe

3397–3398 ↗(On Diff #122724)

This comment maybe belongs to the beginning.

4189–4190 ↗(On Diff #122724)

This is the changed part, right? Under which conditions does addNonEmptyDomainConstraints return nullptr? It's doxygen doesn't mention it.

4354 ↗(On Diff #122724)

No isl::set S?

Edit: From other places I realise you did not convert everything within functions, we do some other time.

lib/CodeGen/IslAst.cpp
678–679 ↗(On Diff #122724)

Did something change here?

lib/CodeGen/IslNodeBuilder.cpp
1074–1075 ↗(On Diff #122724)

Did something change here? clang-format going rogue?

1368 ↗(On Diff #122724)

[Style] no almost-always-auto.

(I only care about introducing new ones, it is up to you if you want to remove existing autos)

1385 ↗(On Diff #122724)

[Style] no almost-always-auto.

lib/Support/GICHelper.cpp
27 ↗(On Diff #122724)

Is this required?

unittests/ScheduleOptimizer/ScheduleOptimizerTest.cpp
15 ↗(On Diff #122724)

Is this required?

This revision is now accepted and ready to land.Nov 14 2017, 7:28 AM

I cannot apply this patch, arcanist always tries to apply it on git sha1 85a1620 (current trunk without D39979).

philip.pfaffe marked 12 inline comments as done.

Address review comments.

Changing the printing from GICHelper towards using to_str() on the isl objects requires changing a handful of testcases.

@Meinersbur: Do you agree with the testcase changes?

Looks great (whatever you decide to to with the inline comment)

lib/Analysis/ScopInfo.cpp
3383 ↗(On Diff #123314)

Could we just remove the dtor? Or declare = default in the header? Is there is a motivation to make it more explicit that there is a default dtor?

This revision was automatically updated to reflect the committed changes.
philip.pfaffe added inline comments.Nov 19 2017, 2:16 PM
lib/Analysis/ScopInfo.cpp
3383 ↗(On Diff #123314)

I just kept it the same as the three other classes in that header/file.

sabuasal added inline comments.
polly/trunk/lib/Analysis/ScopInfo.cpp
2271

How come we are dropping the copy for "Space"?

philip.pfaffe added inline comments.Dec 2 2017, 6:45 AM
polly/trunk/lib/Analysis/ScopInfo.cpp
2271

This is a copy. I'm copying the c++ object.