Page MenuHomePhabricator

[Polly][NFC][ScopBuilder] Move addUserAssumptions to ScopBuilder
ClosedPublic

Authored by domada on Jul 24 2019, 1:23 PM.

Details

Summary

Scope of changes:

  1. Moved addUserAssumptions function to ScopBuilder class.
  2. Moved buildConditionSets functions to polly namespace.
  3. Moved getRepresentingInvariantLoadSCEV to public section of the Scop class

Diff Detail

Repository
rL LLVM

Event Timeline

domada created this revision.Jul 24 2019, 1:23 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: javed.absar. · View Herald Transcript

[remark] Summary: getRepresentingInvariantLoadSCEV is already in the Scop class. Did you want to say changing its visibility to public?

[serious] buildConditionSets is now in the ScopInfo header and therefore made made accessible to the world. This is the opposite of the goal of moving things to ScopBuilder. Is this maybe an intermediate step before also moving buildConditionSets to ScopBuilder?

polly/include/polly/ScopInfo.h
1644 ↗(On Diff #211592)

[nit] Missing space before doxygen comment

polly/lib/Analysis/ScopInfo.cpp
2820 ↗(On Diff #211592)

[nit] Unrelated change

domada edited the summary of this revision. (Show Details)Jul 25 2019, 1:47 PM

[remark] Summary: getRepresentingInvariantLoadSCEV is already in the Scop class. Did you want to say changing its visibility to public?

Indeed. I want to make it public, because I think that it can be hellpful in the future if we want to remove friendship between ScopBuilder and Scop class. I have fixed summary.

[serious] buildConditionSets is now in the ScopInfo header and therefore made made accessible to the world. This is the opposite of the goal of moving things to ScopBuilder. Is this maybe an intermediate step before also moving buildConditionSets to ScopBuilder?

Yes. It is an intermediate step. I have noticed, that buildConditionSets functions are also used by buildDomains which are still inside Scop class. In the next step I want to move buildDomains and buildConditionSets to ScopBuilder.

Maybe you can make a final review when I propose next patch with moved buildDomains functions?

Maybe you can make a final review when I propose next patch with moved buildDomains functions?

Sure

domada updated this revision to Diff 213213.Aug 3 2019, 3:33 PM

Remove duplicated buildConditionSets declaration.

domada added a comment.Aug 4 2019, 2:29 PM

https://reviews.llvm.org/D65729 -> this patch moves buildConditionSets functions to ScopBuilder class. It is not possible to move them enterily in patch 65241 because some Scop functions use these functions.

That's why I have decided to make them visible by adding their declaration to ScopInfo.h file. Patch 65729 solves this dependency and it moves all buildConditionSets functions to ScopBuilder class.

Meinersbur accepted this revision.Aug 5 2019, 3:32 PM

LGTM (with D65729)

This revision is now accepted and ready to land.Aug 5 2019, 3:32 PM
This revision was automatically updated to reflect the committed changes.