[Polly][GSoC 2016]New function pass ScopInfoWrapperPass
ClosedPublic

Authored by cs14mtech11017 on Jun 3 2016, 7:33 AM.

Details

Summary

This patch adds a new function pass ScopInfoWrapperPass so that the polyhedral description of a region, the SCoP,
can be constructed and used in a function pass.

Depends on D20831

Diff Detail

Repository
rL LLVM
cs14mtech11017 retitled this revision from to [Polly][GSoC 2016]New function pass ScopInfoWrapperPass.Jun 3 2016, 7:33 AM
cs14mtech11017 updated this object.
cs14mtech11017 added subscribers: pollydev, llvm-commits.
jdoerfert added inline comments.Jun 6 2016, 3:09 AM
include/polly/ScopInfo.h
2517 ↗(On Diff #59553)

Why top level regions? I don't think that is true.

2519 ↗(On Diff #59553)

what is a "larger pass"? Maybe just replace this by sth like:

This pass is an alternative to the ScopInfoRegionPass in order to avoid a region pass manager.
2531 ↗(On Diff #59553)
RegionToScopMap;
2540 ↗(On Diff #59553)
Scop *getScop(Region *R) const {
  return RegionToScopMap.lookup();
}
2549 ↗(On Diff #59553)

You don't need this function, the above should be enough.

2563 ↗(On Diff #59553)

What's the point of the unique_ptr if we treat them as raw_ptrs anyway?

If I am not mistaken you can remove this loop and only clear the map. The unique_ptrs should automatically call delete on the Scops they point to.

lib/Analysis/ScopInfo.cpp
4876 ↗(On Diff #59553)

I don't like such lines, if nobody is in favour of them I would say we do not introduce them.

4937 ↗(On Diff #59553)

See above.

4962 ↗(On Diff #59553)
for (auto R : SD) { ... }
4968 ↗(On Diff #59553)
RegionToScopMap[R] = SB.getScop();
4974 ↗(On Diff #59553)
It

Please take another look at the naming convention we use.

4987 ↗(On Diff #59553)

polly-scops-all sounds more like "detect unprofitable scops" than "detect scops in a function scope". I would call it -polly-scops-function or -polly-function-scops or somehing similar..

Meinersbur added inline comments.Jun 6 2016, 4:52 AM
include/polly/ScopInfo.h
2556–2557 ↗(On Diff #59553)

You declared a const_iterator type. Can you also define const begin() and end()?

lib/Analysis/ScopInfo.cpp
4876 ↗(On Diff #59553)

I second that.

4951 ↗(On Diff #59553)

Can you use inline declarations when possible?

auto *R = const_cast<Region *>(*I);

Please add some test (e.g., add run lines for this pass).

include/polly/ScopInfo.h
2556–2557 ↗(On Diff #59553)

Or remove the const_iterator.

etherzhhb added inline comments.Jun 6 2016, 9:34 AM
include/polly/ScopInfo.h
2539 ↗(On Diff #59553)

Could we document what happened if a children region of a SCoP region is passed to this function?

Supposed we built Scop S0 for Region R_0, and R_0 has a child (contains) R_1.

What I am getting if I call getScop(R_1)?

2542–2545 ↗(On Diff #59553)

These if-else could written as the following to reduce indent:

if (it != regionToScopMap.end())
  return it->second.get();

return nullptr;
lib/Analysis/ScopInfo.cpp
4968 ↗(On Diff #59553)

Or

bool Inserted = RegionToScopMap.insert(std::make_pair(R, SB.getScop())).second;
assert(Inserted && "Building Scop for the same region twice!");
(void) Inserted;
4974 ↗(On Diff #59553)

Thank you for your inputs. I have updated the patch accordingly.
Please let me know if this set of test cases is not enough.

include/polly/ScopInfo.h
2549 ↗(On Diff #59904)

RegionToScopMap.lookup() does not support unique_ptr.

No technical input. Just wanted to say thank you for everybody helping with reviewing these patches. This really goes very well.

grosser added a subscriber: grosser.
This revision was automatically updated to reflect the committed changes.