This is an archive of the discontinued LLVM Phabricator instance.

[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

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
22

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

39

Unused!

46

Documentation is missing for all these functions.

lib/Analysis/PolyhedralInfo.cpp
38

Do we really require all these passes transitive?

39

Not neeeded.

46

Not neeeded.

54

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

62

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

67

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

72

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

74

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

78

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
100

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

103
for (auto *BB : LI->blocks())
105
auto *Stmt = S->getStmtFor(BB);
if (!Stmt)
  continue;
115

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
39

Removed. Will add as per need.

lib/Analysis/PolyhedralInfo.cpp
38

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

67

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.

101

It was WIP, now updated accordingly.

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

include/polly/PolyhedralInfo.h
24

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

47

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

lib/Analysis/PolyhedralInfo.cpp
53

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

117
if (!SS)
  continue;

lowers the indention

test/Isl/Ast/OpenMP/nested_loop_both_parallel.ll
55

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

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

Maybe reformulate "This interface provides basic interface".

This revision was automatically updated to reflect the committed changes.