Page MenuHomePhabricator

[Polly][NFC][ScopBuilder] Move RecordedAssumptions vector to ScopBuilder.
Needs ReviewPublic

Authored by domada on Sep 25 2019, 3:18 PM.

Details

Summary

Scope of changes:

  1. Moved RecordedAssumptions vector to ScopBuilder. RecordedAssumptions are used only for Scop constructions.
  2. Inside Scop class there are no more calls of function getOrCreateScopArrayInfo. They are replaced by calls of getScopArrayInfo function.
  3. Moved definition of RecordedAssumptionsTy to ScopHelper. It is required both by ScopBuilder and SCEVAffinator.
  4. Add new function recordAssumption to ScopHelper. One of its argument is a reference to RecordedAssumption vector. This function is used by ScopBuilder and SCEVAffinator.
  5. Added reference to RecordedAssumptions to Scop constructor. This reference is forwarded to SCEVAffinator object. It is not more used in Scop class. SCEVAffinator can visit recursively SCEV expressions and it can add record some assumptions.
  6. Removed functions for handling RecordedAssumptions from Scop class.
  7. Removed constness from getScopArrayInfo functions.

Diff Detail

Event Timeline

domada created this revision.Sep 25 2019, 3:18 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: javed.absar. · View Herald Transcript

Nice one.

polly/lib/Analysis/ScopBuilder.cpp
1494–1495

[style] llvm::reverse(RecordedAssumptions) should work as well.

1525

[suggestion] Now that RecordedAssumptions is part of ScopBuilder, it will be freed together with ScopBuilder, so explicitly clearing it should not be necessary.

polly/lib/Analysis/ScopInfo.cpp
699–700

Nice to have this moved out of MemoryAccess

1351

Is this an intentional change? Depending on the iteration order over basic blocks, the value might be used before its write is seem, in which case we'd need getOrCreateScopArrayInfo.

1693

The RecordedAssumptions stored inside ScopBuilder has a shorter lifetime than the Affinator stored inside this Scop object. Meaning that any use of Affinator after the destruction of ScopBuilder will lead to a use-after-free error. It should not happen since adding an assumption after the Scop has been constructed will have no effect. Now new assumptions should be detectable using Valgrind/AddressSanitizer, which might be a good thing, but the potential memory corruption worries me.

Do you have any plans how to ensure no use-after-free happens? E.g. set the RecordedAssumption to nullptr after the Scop has been built. We might even work on even moving the Affinator into ScopBuilder. I would at least appreciate a comment in the code explaining this.

polly/lib/Support/ScopHelper.cpp
155–162

[serious] The comment above applies to simplifyRegion, not recordAssumption.

domada added inline comments.Sep 26 2019, 12:48 PM
polly/lib/Analysis/ScopInfo.cpp
1807

This function is still used in polly/lib/CodeGen/PPCGCodeGeneration.cpp file. That's why I haven't moved it to ScopBuilder.

domada added inline comments.Sep 26 2019, 12:57 PM
polly/lib/Analysis/ScopInfo.cpp
1351

Yes, it was an intentional change. I wanted to move getOrCreateScopArrayInfo to ScopBuilder. Currently this function is used in lib/CodeGen/PPCGCodeGeneration.cpp and in ScopBuilder. That's why I haven't moved it in this patch. I need to rethink this change.

1693

Thank you for your comment. Initially I will set RecordedAssumption to nullptr. Finally I will try to move Affinator into ScopBuilder. I think that it should be the most elegant way.