This is an archive of the discontinued LLVM Phabricator instance.

[GSoC 2016][Polly][Refactor] Decouple SCoP building logic from pass
ClosedPublic

Authored by cs14mtech11017 on May 28 2016, 4:17 AM.

Details

Summary

Decoupled SCoP creation and pass logic.

  1. Created a new pass ScopInfoRegionPass. As name suggests, it is a region pass and it is there to preserve compatibility with our existing Polly passes.
  2. ScopInfoRegionPass will return ScopInfo object, which actually builds the SCoP object.

Diff Detail

Repository
rL LLVM

Event Timeline

cs14mtech11017 retitled this revision from to [GSoC 2016][Polly][Refactor] Decouple SCoP building logic from pass.
cs14mtech11017 updated this object.
cs14mtech11017 added reviewers: Unknown Object (User), grosser, Meinersbur, etherzhhb.
cs14mtech11017 added a subscriber: Restricted Project.
cs14mtech11017 edited reviewers, added: jdoerfert; removed: Unknown Object (User).May 28 2016, 6:24 AM
cs14mtech11017 added a subscriber: pollydev.
jdoerfert edited edge metadata.May 30 2016, 2:27 AM

I added some comments and change requests. Additionally, could you use "arc diff" to create a revision or alternatively add "-U99999" to the "diff" options such that we get context between the chunks that were changes? Thanks!

include/polly/ScopInfo.h
2478 ↗(On Diff #58888)

I wondered why this method takes arguments already passed to the constructor and I needed to check the code. Two things:

  1. Please add a field for the AssumptionCache reference.
  2. The createScopInfo() function should be either: a) private and called "initialize" or "init" b) inlined into the constructor
2482 ↗(On Diff #58888)

Remove the virtual please.

2498 ↗(On Diff #58888)

I don't know why you need a unique_ptr here but I don't care to much about it.

2510 ↗(On Diff #58888)

I would not return a reference here because there is no way to know that SI is actually not null. Please return the SI pointer.

lib/Analysis/ScopInfo.cpp
4869 ↗(On Diff #58888)

can you move this check to the runOnRegion method below?

lib/Analysis/ScopPass.cpp
23 ↗(On Diff #58888)

I would change the interface such that getScopInfo might return a nullptr. At the moment I don't care if ScopInfo.getScop() returns a pointer or a reference but the latter might be possible.

cs14mtech11017 updated this object.
cs14mtech11017 edited edge metadata.
  • Decouple Scop building logic from pass logic
  • Removed createScopInfo function and inlined code into ScopInfo constructor.

I have done the changes you requested. Have arc for the new patch.

include/polly/ScopInfo.h
2476–2478 ↗(On Diff #58888)

Ok, removed this function and inlined code into the constructor.

2482 ↗(On Diff #58888)

Removed. thanks for pointing that out.

2498 ↗(On Diff #58888)

As this is an analysis pass, I think it is better for memory management. Don't have to track all the pointer uses.

2510 ↗(On Diff #58888)

Ok. After making the change at 4896, this makes sense.

lib/Analysis/ScopInfo.cpp
4869 ↗(On Diff #58888)

Done. Also removed this function and inlined code into constructor.

The missing print will fix the last unit test. Other than that LGTM. Tobias announced comments, thus let's wait for him.

lib/Analysis/ScopInfo.cpp
4920 ↗(On Diff #58943)
else
  OS << "Invalid Scop!\n";
grosser edited edge metadata.May 30 2016, 5:10 AM

Hi Utpal,

thank you for looking into this! As Johannes said, the patch looks generally good. I mostly have minor comments.

Also, I would suggest to look after this commit into the memory management of ScopInfo/ScopBuilder. It seems unnecessary to keep it around after the scop was constructed just to keep a pointer to the scop. I would prefer to just have it return the scop object and (or a unique_ptr to it) and not keep any state after the scop has been constructed.

Best,
Tobias

include/polly/ScopInfo.h
2250 ↗(On Diff #58943)

Would it make sense to call this ScopBuilder? This seems more descriptive.

You can probably do this in a follow-up commit.

2476 ↗(On Diff #58943)

The releaseMemory method does not seem to be needed any more, no?

2478 ↗(On Diff #58943)

Why is a print method needed here? We could just get the scop and print it directly, no?

lib/Analysis/ScopPass.cpp
23 ↗(On Diff #58943)

Johannes, I do not understand this comment/change? Why do we need to expose ScopInfo/ScopBuilder? Either getScop() returns a scop or a nullptr? That seems all the functionality we need, right?

Also, I would suggest to look after this commit into the memory management of ScopInfo/ScopBuilder. It seems unnecessary to keep it around after the scop was constructed just to keep a pointer to the scop. I would prefer to just have it return the scop object and (or a unique_ptr to it) and not keep any state after the scop has been constructed.

We can do that now or afterwards but it sounds like a good idea.

lib/Analysis/ScopPass.cpp
23 ↗(On Diff #58943)

Ok, fair enough. We can get rid of the indirection completely (same argument as I used to justify the reference return argument for ScopInfo.getScop(), once a ScopInfo/ScopBuilder is created there should always be a Scop).

@Utpal, can you please replace ScopInfoRegionPass::getScopInfo with a ScopInfoRegionPasS::getScop function?

cs14mtech11017 added inline comments.May 30 2016, 6:04 AM
include/polly/ScopInfo.h
2250 ↗(On Diff #58943)

I was in the intuition of making least possible changes to the existing code base.
Please suggest changes that you feel will be better in general.

If I am not wrong, we do not need to change the files names of ScopInfo.h and ScopInfo.cpp right? Changing only class name is makes sense.

cs14mtech11017 added inline comments.

Comment at: include/polly/ScopInfo.h:2250
@@ -2249,3 +2249,3 @@
/// @brief Build the Polly IR (Scop and ScopStmt) on a Region.
-class ScopInfo : public RegionPass {

//===-------------------------------------------------------------------===//

grosser wrote:

Would it make sense to call this ScopBuilder? This seems more descriptive.

You can probably do this in a follow-up commit.

I was in the intuition of making least possible changes to the existing code base.
Please suggest changes that you feel will be better in general.

Yes. As this renaming is purely mechanical it is probably better to do
it in a subsequent change.

If I am not wrong, we do not need to change the files names of ScopInfo.h and ScopInfo.cpp right? Changing only class name is makes sense.

No. I don't think we need to.

Best,
Tobias

cs14mtech11017 edited edge metadata.
  • Removed releaseMemory and print functions from ScopInfo class as they are not required any more.

Hello Tobias,

Most of the changes you suggested are done. Removed releaseMemory and print functions, removed exposing ScopInfo object and now returning just the scop object.
I will push class renaming change from ScopInfo to ScopBuilder and memory management changes in a separate patch.

grosser accepted this revision.May 30 2016, 8:47 AM
grosser edited edge metadata.

LGTM.

This revision is now accepted and ready to land.May 30 2016, 8:47 AM
This revision was automatically updated to reflect the committed changes.