This is an archive of the discontinued LLVM Phabricator instance.

[Polly][NFC][ScopBuilder] Move RecordedAssumptions vector to ScopBuilder.
ClosedPublic

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. Moved definition of RecordedAssumptionsTy to ScopHelper. It is required both by ScopBuilder and SCEVAffinator.
  3. Add new function recordAssumption to ScopHelper. One of its argument is a reference to RecordedAssumption vector. This function is used by ScopBuilder and SCEVAffinator.
  4. All RecordedAssumptions are created by ScopBuilder. isl::pw_aff objects for corresponding SCEVs are created inside ScopBuilder. Scop functions do not record any assumptions. Scop can use isl::pw_aff objects which were created by ScopBuilder.
  5. Removed functions for handling RecordedAssumptions from Scop class.
  6. Removed constness from getScopArrayInfo functions.
  7. Replaced SCEVVisitor struct from SCEVAffinator with taylored version, which allow to pass pointer to RecordedAssumptions as function argument.

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.

domada updated this revision to Diff 232935.Dec 9 2019, 1:54 PM
domada edited the summary of this revision. (Show Details)
  1. Reworked mechanism of collecting assumptions. All assumptions are now taken inside ScopBuilder.
  2. Internal functions of Scop class do not record any assumption. They use cached isl::pw_aff objects which are created by ScopBuilder.
domada marked 3 inline comments as done.Dec 9 2019, 2:11 PM
domada added inline comments.
polly/lib/Analysis/ScopInfo.cpp
1693

Initial change is reverted.

Could you upload the nest diff with context (git diff -U999999)?

polly/include/polly/ScopBuilder.h
74

[style] RecordedAssumptionsTy RecordedAssumptions = nullptr preferable over initializer in the ctor.

polly/lib/Analysis/ScopBuilder.cpp
3230

[style] No Almost Always Auto: for (const SCEV *Size: Access->Sizes)

[style] Variable start with upper cases.

3241

[style] No Almost Always Auto: for (const SCEV *Subscript : Access->Subscripts)

[suggestion] Access using a getter might be preferable

3799

[suggestion] Clearing may not be necessary.

polly/lib/Support/SCEVAffinator.cpp
120

[serious] Assigning the field in one of the methods instead of in the ctor makes the lifetime issue even worse. Is there a reason not to pass RecordedAssumptions by function arguments?

domada updated this revision to Diff 234389.Dec 17 2019, 2:27 PM

Uploaded the same changes with the more context: git format-patch -U999999 HEAD~1

domada added inline comments.Dec 17 2019, 3:03 PM
polly/lib/Support/SCEVAffinator.cpp
120

Some RecordedAssumptions are added inside visit and other visit_type_of_scev functions functions (for example visitTruncateExpr ). visit function and other visit_type_of_scev functions are part of` llvm::SCEVVisitor` struct. I cannot modify number of parameters for these functions, because they are part of LLVM infrastructure.

That's why I am trying to add pointer of RecordedAssumption vector to SCEVAffinator. This pointer is visible inside all functions inside SCEVAffinator class and I don't need to pass it as function argument.

I am wondering if it is worth to split SCEVAffinator into two classes? The first class will be responsible only for delivering PWACtx, and the second will be responsible for recording assumptions. The class for collecting assumptions will be instantiated only in ScopBuilder so the problem with object lifetime will be solved. The drawback of this approach is duplicated traversing of some SCEVs.

Meinersbur accepted this revision.Dec 18 2019, 1:52 PM

LGTM. Please consider the [style] remarks as well before committing.

Thanks for the effort.

polly/lib/Support/SCEVAffinator.cpp
120

I see, thanks for the explanation.

For the [[ https://github.com/llvm/llvm-project/blob/master/polly/lib/Transform/ScheduleTreeTransform.cpp#L25 | ScheduleTreeVisitor ]] my solution was to implement visitor methods with variadic arguments, so you actually can pass additional parameters.

But I also think any workaround will be overkill for now and we just use the solution already done for BB.

This revision is now accepted and ready to land.Dec 18 2019, 1:52 PM
domada updated this revision to Diff 234630.Dec 18 2019, 3:09 PM
domada edited the summary of this revision. (Show Details)

This patch implements individual version of SCEVVisitor. Taylored version of SCEVVisitor is required to pass pointer to RecordedAssumptions to visit_type_scev functions.

Please let me know if you prefer this solution or the previous one.

Before commit I will also fix style remarks.

domada added inline comments.Dec 18 2019, 3:21 PM
polly/lib/Support/SCEVAffinator.cpp
120

I have implemented something similar to ScheduleTreeVisitor, please look at function visitSCEV and please express your opinion.

On one hand implementation of own SCEVVisitor resolves problem of lifetime issue, but on the other hand I have created more code which is to some extent redundant to LLVM code.

This revision was automatically updated to reflect the committed changes.