This is an archive of the discontinued LLVM Phabricator instance.

[Polly][GSoC 2016]Update ScopBuilder's memory management
ClosedPublic

Authored by cs14mtech11017 on Jun 2 2016, 8:25 AM.

Details

Summary

This patch updates memory management of ScopBuilder class.

  1. SCoP object is not owned by ScopBuilder. It just creates a SCoP and hand over ownership through getScop() method.
  2. ScopInfoRegionPass owns the SCoP object for a given region.

Depends on D20831

Diff Detail

Event Timeline

cs14mtech11017 retitled this revision from to [Polly][GSoC 2016]Update ScopBuilder's memory management.
cs14mtech11017 updated this object.
cs14mtech11017 added subscribers: pollydev, llvm-commits.
jdoerfert edited edge metadata.Jun 2 2016, 10:12 AM

please rebase such that the rename happened.

cs14mtech11017 updated this object.
cs14mtech11017 edited edge metadata.

Rebased over patch D20831 which is not committed yet.

etherzhhb added inline comments.Jun 2 2016, 3:00 PM
lib/Analysis/ScopInfo.cpp
4901

New without delete?

sebpop added a subscriber: sebpop.Jun 2 2016, 3:09 PM
sebpop added inline comments.
include/polly/ScopInfo.h
2485

Variable names should start with capital letters: "Scop".

Follow variable naming convention.
Delete ScopBuilder after new in ScopInfoRegionPass::runOnRegion.

Meinersbur added inline comments.Jun 3 2016, 5:17 AM
include/polly/ScopInfo.h
2485

'Scop' is already the name of the class; using it for variable names leads to disambiguation problems in the C++ parser.

I suggest to use just 'S' or invent something else. e.g. 'Part' (What the P stands for in Scop).

jdoerfert added inline comments.Jun 3 2016, 5:47 AM
lib/Analysis/ScopInfo.cpp
4901

Why allocate the ScopBuilder on the heap anyway?
Doesn't this work:

ScopBuilder SB(R, AC, AA, DL, DT, LI, SD, SE);
cs14mtech11017 added inline comments.Jun 3 2016, 6:32 AM
lib/Analysis/ScopInfo.cpp
4901

Yes this would be even better. Thanks.

Please review and comment.

grosser added a subscriber: grosser.

Dear Utpal,

thanks for your contribution. I agree with the direction and Johannes has the more-details review under control, so I leave this to him.

Best,
Tobias

jdoerfert accepted this revision.Jun 13 2016, 8:11 AM
jdoerfert edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jun 13 2016, 8:11 AM
This revision was automatically updated to reflect the committed changes.