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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur 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:
> 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?


https://reviews.llvm.org/D31719