[PATCH] D68056: [Polly][NFC][ScopBuilder] Move RecordedAssumptions vector to ScopBuilder.

[Dominik Adamski via Phabricator via llvm-commits]

domada added inline comments.


================
Comment at: polly/lib/Support/SCEVAffinator.cpp:120
   this->BB = BB;
+  this->RecordedAssumptions = RecordedAssumptions;
 
----------------
Meinersbur wrote:
> domada wrote:
> > Meinersbur wrote:
> > > [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?
> > 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.
> 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`.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68056/new/

https://reviews.llvm.org/D68056