[Polly][GSoC 2016]Add PolyhedralInfo pass- new interface to Polly Analysis

Authored by cs14mtech11017 on Jun 17 2016, 5:21 PM.



Adding a new pass PolyhedralInfo. This pass will be the interface to Polly.
Initially, we will provide the following interfaces, which could be extended at a later phase:-
#IsParallel(Loop *L) - return a bool depending on whether the loop is parallel or not for the given program order.
#IsVectorizable(Loop *L) - return minimum dependence distance for the loop. This is work in progress.

Please provide your suggestions and directions so that I can take a better approach.

Diff Detail

cs14mtech11017 retitled this revision from to [Polly][GSoC 2016][WIP]Add PolyhedralInfo pass- new interface to Polly Analysis.Jun 17 2016, 5:21 PM
cs14mtech11017 updated this object.
cs14mtech11017 added a subscriber: grosser.

Some comments are inlined. Also add test cases please.

21 ↗(On Diff #61146)

Please do not open the namespace but use llvm:: where needed.

38 ↗(On Diff #61146)


45 ↗(On Diff #61146)

Documentation is missing for all these functions.

37 ↗(On Diff #61146)

Do we really require all these passes transitive?

38 ↗(On Diff #61146)

Not neeeded.

45 ↗(On Diff #61146)

Not neeeded.

53 ↗(On Diff #61146)

Maybe also print the "name" of the loop not only if it is parallel or not.

61 ↗(On Diff #61146)

Move this down before the conditional where it is possibly changed.

66 ↗(On Diff #61146)

Maybe change the "getDependences" interface to accept const Scops first.

71 ↗(On Diff #61146)

If you do not include reduction dependences here you need to make that clear in the documentation.

73 ↗(On Diff #61146)

Does this work? I would have expected you would get the initial schedule from the Scop.

77 ↗(On Diff #61146)

Why do you check isParallel twice, you can simply add the TC_RED dependences above.

cs14mtech11017 marked 8 inline comments as done.

More comments

99 ↗(On Diff #62630)

I would not declare variables early if they have an actual limited scope. The two comments below show I would declare them.

102 ↗(On Diff #62630)
for (auto *BB : LI->blocks())
104 ↗(On Diff #62630)
auto *Stmt = S->getStmtFor(BB);
if (!Stmt)
114 ↗(On Diff #62630)

I just recalled that you can use the Scop::getRelativeLoopDepth(Loop *) function here too.

Fixed corrupt memory issues due to isl objects. Added test cases.

cs14mtech11017 marked 4 inline comments as done.Jul 8 2016, 6:29 AM

Updated the patch with necessary changes. Added unit test cases.

38 ↗(On Diff #61146)

Removed. Will add as per need.

100 ↗(On Diff #63213)

It was WIP, now updated accordingly.

37 ↗(On Diff #61146)

DependenceInfoWrapperPass and ScopInfoWrapperPass are required transitive. LoopInfoWrapperPass is not. Updated accordingly.

66 ↗(On Diff #61146)

Anyhow we will require a constant cast. Dependences::calculateDependences(Scop &S) is being called from DependenceInfoWrapperPass. It is better to do it in this interface itself.

Some minor remarks but after they are fixed it looks good to go in.

23 ↗(On Diff #63213)

Please do not open the namespace but use llvm:: where needed.

46 ↗(On Diff #63213)

"in its outermost dimension" doesn't make sense. Just remove that part.

52 ↗(On Diff #63213)

"Loop" is a bad variable name as it is a type, use L or something else.

116 ↗(On Diff #63213)
if (!SS)

lowers the indention

55 ↗(On Diff #63213)

Why is there a check not omp parallel for but you report a parallel loop?

cs14mtech11017 marked 5 inline comments as done.

Updated according to review comments.

cs14mtech11017 added inline comments.Jul 11 2016, 9:20 AM
55 ↗(On Diff #63523)

Its a restriction on nested openmp parallel pragma where the outer loop is also openmp parallel.

cs14mtech11017 retitled this revision from [Polly][GSoC 2016][WIP]Add PolyhedralInfo pass- new interface to Polly Analysis to [Polly][GSoC 2016]Add PolyhedralInfo pass- new interface to Polly Analysis.Jul 15 2016, 4:16 AM

Does this pass LNT when it is part of the Polly pipeline [profitable but also unprofitable] or are there any crashes? If not I would go ahead and commit it for you.

I tested this with lnt and there are no crashes. However, there exists few failed cases with and without this patch.

This revision is now accepted and ready to land.Jul 15 2016, 7:53 AM
mreisinger added inline comments.
15 ↗(On Diff #64124)

Maybe reformulate "This interface provides basic interface".

This revision was automatically updated to reflect the committed changes.