[RegionInfo] Fix dangling references created by moving RegionInfo objects

[philip.pfaffe]

Summary

Region objects capture the address of the creating RegionInfo instance. Because the RegionInfo class is movable, moving a RegionInfo object creates dangling references. This patch fixes these references by walking the Regions post-move, and updating references to the new parent.

[grosser]

LGTM.

[philip.pfaffe]

Simplify the patch slightly. NFC.

[Meinersbur]

include/llvm/Analysis/RegionInfo.h

896–903 ↗

(On Diff #94349)

Would it be feasible to remove the move-ctors and instead force users to allocate RegionInfo on the heap?

[Meinersbur]

include/llvm/Analysis/RegionInfo.h

896–903 ↗

(On Diff #94349)

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?

[philip.pfaffe]

include/llvm/Analysis/RegionInfo.h

896–903 ↗

(On Diff #94349)

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.

[grosser]

include/llvm/Analysis/RegionInfo.h

896–903 ↗

(On Diff #94349)

AFAIR the getRegionInfo() function is not used very often, so if we can get rid of it entirely that would be nice.

Now, I believe this is an orthogonal issue. Meaning, the patch as written solves the direct correctness issue and seems consequently worth committing. If we can then also simplify the interface, this is certainly something I would be glad about.

llvm/trunk/include/llvm/Analysis/RegionInfo.h

class RegionInfoBase {
 
   DomTreeT *DT;
   PostDomTreeT *PDT;
   DomFrontierT *DF;
 
   /// The top level region.
   RegionT *TopLevelRegion;
 
-private:
   /// Map every BB to the smallest region, that contains BB.
   BBtoRegionMap BBtoRegion;
 
+protected:
+  /// \brief Update refences to a RegionInfoT held by the RegionT managed here
+  ///
+  /// This is a post-move helper. Regions hold references to the owning
+  /// RegionInfo object. After a move these need to be fixed.
+  template<typename TheRegionT>
+  void updateRegionTree(RegionInfoT &RI, TheRegionT *R) {
+    if (!R)
+      return;
+    R->RI = &RI;
+    for (auto &SubR : *R)
+      updateRegionTree(RI, SubR.get());
+  }
+
+private:
   /// \brief Wipe this region tree's state without releasing any resources.
   ///
   /// This is essentially a post-move helper only. It leaves the object in an
   /// assignable and destroyable state, but otherwise invalid.
   void wipe() {
     DT = nullptr;
     PDT = nullptr;
     DF = nullptr;

 class RegionInfo : public RegionInfoBase<RegionTraits<Function>> {
 public:
   typedef RegionInfoBase<RegionTraits<Function>> Base;
 
   explicit RegionInfo();
 
   ~RegionInfo() override;
 
-  RegionInfo(RegionInfo &&Arg)
-    : Base(std::move(static_cast<Base &>(Arg))) {}
+  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;
   }
 
   /// Handle invalidation explicitly.
   bool invalidate(Function &F, const PreservedAnalyses &PA,
                   FunctionAnalysisManager::Invalidator &);
 
   // updateStatistics - Update statistic about created regions.