Page MenuHomePhabricator

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

Authored by domada on Jun 19 2019, 2:20 PM.

Details

Summary

Scope of changes:

  1. Moved addRecordedAssumptions to ScopBuilder class.
  2. Moved Assumption struct outside Scop class.
  3. Refactored addRecordedAssumptions function. Replaced while loop by for_range loop.
  4. Added function to clear processed Assumptions.

Diff Detail

Repository
rL LLVM

Event Timeline

domada created this revision.Jun 19 2019, 2:20 PM
domada updated this revision to Diff 205689.Jun 19 2019, 3:35 PM
  1. Remove unused function.

[suggestion] RecordedAssumptions itself is only used during SCoP constructions (since it's empty once the SCoP is full constructed). It could be removed entirely into ScopBuilder, maybe in a future patch?

polly/lib/Analysis/ScopBuilder.cpp
391 ↗(On Diff #205689)

Please remove commented-out code entirely.

domada updated this revision to Diff 206745.Jun 26 2019, 2:18 PM
  1. Removed commented code
  2. Renamed iterator range to RecordedAssumptions to recorded_assumptions
domada marked an inline comment as done.EditedJun 26 2019, 2:20 PM

Ok, I will move entirely RecordedAssumptions to ScopBuilder in a future patch.

Meinersbur accepted this revision.Jul 10 2019, 10:49 AM

LGTM

polly/include/polly/ScopInfo.h
2344 ↗(On Diff #206745)

[suggestion] Add const qualifier

2345 ↗(On Diff #206745)

Why reverse iterator? The user could also use llvm::reverse

This revision is now accepted and ready to land.Jul 10 2019, 10:49 AM
domada added inline comments.Jul 10 2019, 3:32 PM
polly/include/polly/ScopInfo.h
2345 ↗(On Diff #206745)

I was not aware of llvm::reverse function.

I needed reverse iterator for refactored addRecordedAssumptions function. I will fix this issue. Should I update review request or can I fix it when I will commit?

Meinersbur added inline comments.Jul 11 2019, 10:05 AM
polly/include/polly/ScopInfo.h
2345 ↗(On Diff #206745)

I do sometimes the diff with a "Update before commit (+ changes)" message; Phabricator 'complains' with a "Changed prior to commit:" line if it thinks the commit and the patch are not identical (but it's not reliable).

I'd say, update the review if you are unsure about how you resolved the pending comments, but it is not required.

domada updated this revision to Diff 209963.Jul 15 2019, 3:04 PM

Applied @Meinersbur remarks.

domada marked 2 inline comments as done.Jul 15 2019, 3:06 PM
This revision was automatically updated to reflect the committed changes.