[ConstantHoisting] Call cleanup() in ConstantHoistingPass::runImpl to avoid dangling elements in ConstIntInfoVec for new PM

[MaskRay]

Summary

ConstIntInfoVec contains elements extracted from the previous function.
In new PM, releaseMemory() is not called and the dangling elements can
cause segfault in findConstantInsertionPoint.

Rename releaseMemory() to cleanup() to deliver the idea that it is
mandatory and call cleanup() in ConstantHoistingPass::runImpl to fix
this.

[ormris]

lib/Transforms/Scalar/ConstantHoisting.cpp

948

If this only happens in the new pass manager, could this call be moved to ::run? That way, the legacy pass manager doesn't call this function twice.

[ormris]

lib/Transforms/Scalar/ConstantHoisting.cpp

948

s/::run/ConstantHoistingPass::run/g

[MaskRay]

Remove releaseMemory() and inline it into runImpl()

[MaskRay]

lib/Transforms/Scalar/ConstantHoisting.cpp

948

Thanks! I think maybe it makes more sense to delete releaseMemory() and inline it into the end of runImpl. I've updated the patch to do that.

[MaskRay]

Update title

[ormris]

LGTM. I would wait for one other review before committing.

[wmi]

lib/Transforms/Scalar/ConstantHoisting.cpp

948

nit: why you feel it is better to inline it? It seems better to me to extract those cleanups into a function.

[MaskRay]

lib/Transforms/Scalar/ConstantHoisting.cpp

948

releaseMemory() is called by legacy PM but not by new PM. This releaseMemory() doesn't do what people expected: this is not only related to release unused memory but also related to correctness. The data structure should be destroyed after applying on one function as otherwise it would become dangling.

[MaskRay]

Rename releaseMemory() to cleanup() as wmi suggested.

[wmi]

LGTM

[MaskRay]

Thank you for review!

include/llvm/Transforms/Scalar/ConstantHoisting.h

 class ConstantHoistingPass : public PassInfoMixin<ConstantHoistingPass> {
 public:
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
 
   // Glue for old PM.
   bool runImpl(Function &F, TargetTransformInfo &TTI, DominatorTree &DT,
                BlockFrequencyInfo *BFI, BasicBlock &Entry);
 
-  void releaseMemory() {
+  void cleanup() {
     ClonedCastMap.clear();
     ConstIntCandVec.clear();
     for (auto MapEntry : ConstGEPCandMap)
       MapEntry.second.clear();
     ConstGEPCandMap.clear();
     ConstIntInfoVec.clear();
     for (auto MapEntry : ConstGEPInfoMap)
       MapEntry.second.clear();

lib/Transforms/Scalar/ConstantHoisting.cpp

public:
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     if (ConstHoistWithBlockFrequency)
       AU.addRequired<BlockFrequencyInfoWrapperPass>();
     AU.addRequired<DominatorTreeWrapperPass>();
     AU.addRequired<TargetTransformInfoWrapperPass>();
   }
 
-  void releaseMemory() override { Impl.releaseMemory(); }
-
 private:
   ConstantHoistingPass Impl;
 };
 
 } // end anonymous namespace
 
 char ConstantHoistingLegacyPass::ID = 0;
 
bool ConstantHoistingPass::runImpl(Function &Fn, TargetTransformInfo &TTI,
   for (auto MapEntry : ConstGEPInfoMap)
     if (!MapEntry.second.empty())
       MadeChange |= emitBaseConstants(MapEntry.first);
 
 
   // Cleanup dead instructions.
   deleteDeadCastInst();
 
+  cleanup();
+
   return MadeChange;
 }
 
 PreservedAnalyses ConstantHoistingPass::run(Function &F,
                                             FunctionAnalysisManager &AM) {
   auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
   auto &TTI = AM.getResult<TargetIRAnalysis>(F);
   auto BFI = ConstHoistWithBlockFrequency

test/Transforms/ConstantHoisting/X86/phi.ll

 ; RUN: opt -S -consthoist < %s | FileCheck %s
+; RUN: opt -S -passes=consthoist < %s | FileCheck %s
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.9.0"
 
 ; PR18626
 define i8* @test1(i1 %cmp, i64* %tmp) {
 entry:
   call void @foo(i8* inttoptr (i64 68719476735 to i8*))