Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
rL LLVM

Event Timeline

cs14mtech11017 retitled this revision from to [Polly][GSoC 2016][WIP]Add PolyhedralInfo pass- new interface to Polly Analysis.
cs14mtech11017 updated this object.
cs14mtech11017 added a subscriber: grosser.
jdoerfert edited edge metadata.Jun 20 2016, 5:20 AM

Some comments are inlined. Also add test cases please.

include/polly/PolyhedralInfo.h
21 ↗(On Diff #61146)

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

38 ↗(On Diff #61146)

Unused!

45 ↗(On Diff #61146)

Documentation is missing for all these functions.

lib/Analysis/PolyhedralInfo.cpp
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 edited edge metadata.
cs14mtech11017 marked 8 inline comments as done.

More comments

lib/Analysis/PolyhedralInfo.cpp
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)
  continue;
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.

include/polly/PolyhedralInfo.h
38 ↗(On Diff #61146)

Removed. Will add as per need.

lib/Analysis/PolyhedralInfo.cpp
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.

include/polly/PolyhedralInfo.h
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.

lib/Analysis/PolyhedralInfo.cpp
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)
  continue;

lowers the indention

test/Isl/Ast/OpenMP/nested_loop_both_parallel.ll
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
test/Isl/Ast/OpenMP/nested_loop_both_parallel.ll
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.

jdoerfert accepted this revision.Jul 15 2016, 7:53 AM
jdoerfert edited edge metadata.

LGTM.

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

Maybe reformulate "This interface provides basic interface".

This revision was automatically updated to reflect the committed changes.