[PATCH] D31719: [RegionInfo] Fix dangling references created by moving RegionInfo objects

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added inline comments.


================
Comment at: include/llvm/Analysis/RegionInfo.h:896-903
+  RegionInfo(RegionInfo &&Arg) : Base(std::move(static_cast<Base &>(Arg))) {
+    updateRegionTree(*this, TopLevelRegion);
+  }
   RegionInfo &operator=(RegionInfo &&RHS) {
     Base::operator=(std::move(static_cast<Base &>(RHS)));
+    updateRegionTree(*this, TopLevelRegion);
     return *this;
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > Would it be feasible to remove the move-ctors and instead force users to allocate RegionInfo on the heap?
> As `LoopInfo` is also a movable type, this might be required by the new pass manager.
> 
> Looking at the implementation of `AnalysisManager`, analysis are stored as
> ```
>   typedef std::list<std::pair<AnalysisKey *, std::unique_ptr<ResultConceptT>>>
>       AnalysisResultListT;
> ```
> that is, it only has ownership, but does not require it to be movable.
> 
> On the other side, the unique_ptr is created using
> ```
>   std::unique_ptr<
>       AnalysisResultConcept<IRUnitT, PreservedAnalysesT, InvalidatorT>>
>   run(IRUnitT &IR, AnalysisManager<IRUnitT, ExtraArgTs...> &AM,
>       ExtraArgTs... ExtraArgs) override {
>     return llvm::make_unique<ResultModelT>(Pass.run(IR, AM, ExtraArgs...));
>   }
> ```
> in `PassManagerInternals.h`. It is effectively used by `bool registerPass(PassBuilderT &&PassBuilder)`.
> Passing an analysis result object by value, only to wrap it into a unique_ptr afterwards seems wasteful. The comment for the `AnalysisPassModel` class mentions that it is only a wrapper. It should be possible to implement an `AnalysisPassConcept` without the wrapper, but there is no existing implementation doing that.
> 
> I don't expect you to explore this unknown territory. Could you maybe add a comment to the source mentioning why the analysis result needs to be moved so it will be considered when someone is going to improve the situation?
The fundamental difference between `LoopInfo` and `RegionInfo` in this regard is that `Loop` does not hold any references to `LoopInfo`. It might be worthwhile to move the `RegionInfo` and `Region` API more towards `LoopInfo`, meaning to remove the captured reference from the `Region`-class and pass it in the relevant functions instead.

> The comment for the AnalysisPassModel class mentions that it is only a wrapper.
That's not how the PassManager is supposed to be used, and that won't change in the future. The explicit design decision is that Analyses and Passes do not depend on PassManager internals, such as deriving from an interface- or base-class. That is precisely the idea of concept-based programming.





https://reviews.llvm.org/D31719