[Polly][GSoC 2016]Update ScopBuilder's memory management

[cs14mtech11017]

Summary

This patch updates memory management of ScopBuilder class.

1. SCoP object is not owned by ScopBuilder. It just creates a SCoP and hand over ownership through getScop() method.
2. ScopInfoRegionPass owns the SCoP object for a given region.

Depends on D20831

[jdoerfert]

please rebase such that the rename happened.

[cs14mtech11017]

Rebased over patch D20831 which is not committed yet.

[etherzhhb]

lib/Analysis/ScopInfo.cpp

4903 ↗

(On Diff #59432)

New without delete?

[sebpop]

include/polly/ScopInfo.h

2488 ↗

(On Diff #59432)

Variable names should start with capital letters: "Scop".

[cs14mtech11017]

Follow variable naming convention.
Delete ScopBuilder after new in ScopInfoRegionPass::runOnRegion.

[Meinersbur]

include/polly/ScopInfo.h

2488 ↗

(On Diff #59432)

'Scop' is already the name of the class; using it for variable names leads to disambiguation problems in the C++ parser.

I suggest to use just 'S' or invent something else. e.g. 'Part' (What the P stands for in Scop).

[jdoerfert]

lib/Analysis/ScopInfo.cpp

4901 ↗

(On Diff #59534)

Why allocate the ScopBuilder on the heap anyway?
Doesn't this work:

ScopBuilder SB(R, AC, AA, DL, DT, LI, SD, SE);

[cs14mtech11017]

lib/Analysis/ScopInfo.cpp

4901 ↗

(On Diff #59534)

Yes this would be even better. Thanks.

[cs14mtech11017]

Please review and comment.

[grosser]

Dear Utpal,

thanks for your contribution. I agree with the direction and Johannes has the more-details review under control, so I leave this to him.

Best,
Tobias

[jdoerfert]

LGTM.

polly/trunk/include/polly/ScopInfo.h

class ScopBuilder {
   SmallVector<Instruction *, 16> GlobalReads;
 
   /// @brief Set of all accessed array base pointers.
   SmallSetVector<Value *, 16> ArrayBasePointers;
 
   // The Scop
   std::unique_ptr<Scop> scop;
 
-  // Clear the context.
-  void clear();
-
   // Build the SCoP for Region @p R.
   void buildScop(Region &R, AssumptionCache &AC);
 
   /// @brief Try to build a multi-dimensional fixed sized MemoryAccess from
   ///        the Load/Store instruction.
   ///
   /// @param Inst       The Load/Store instruction that access the memory
   /// @param L          The parent loop of the instruction
public:
   explicit ScopBuilder(Region *R, AssumptionCache &AC, AliasAnalysis &AA,
                        const DataLayout &DL, DominatorTree &DT, LoopInfo &LI,
                        ScopDetection &SD, ScalarEvolution &SE);
   ~ScopBuilder() {}
 
   /// @brief Try to build the Polly IR of static control part on the current
   ///        SESE-Region.
   ///
-  /// @return If the current region is a valid for a static control part,
-  ///         return the Polly IR representing this static control part,
-  ///         return null otherwise.
-  Scop *getScop() { return scop.get(); }
-  const Scop *getScop() const { return scop.get(); }
+  /// @return Give up the ownership of the scop object or static control part
+  ///         for the region
+  std::unique_ptr<Scop> getScop() { return std::move(scop); }
 };
 
 /// @brief The legacy pass manager's analysis pass to compute scop information
 ///        for a region.
 class ScopInfoRegionPass : public RegionPass {
-  /// @brief The ScopBuilder pointer which is used to construct a Scop.
-  std::unique_ptr<ScopBuilder> SI;
+  /// @brief The Scop pointer which is used to construct a Scop.
+  std::unique_ptr<Scop> S;
 
 public:
   static char ID; // Pass identification, replacement for typeid
 
   ScopInfoRegionPass() : RegionPass(ID) {}
   ~ScopInfoRegionPass() {}
 
-  /// @brief Build ScopBuilder object, which constructs Polly IR of static
-  ///        control part for the current SESE-Region.
+  /// @brief Build Scop object, the Polly IR of static control
+  ///        part for the current SESE-Region.
   ///
-  /// @return Return Scop for the current Region.
-  Scop *getScop() {
-    if (SI)
-      return SI.get()->getScop();
-    else
-      return nullptr;
-  }
-  const Scop *getScop() const {
-    if (SI)
-      return SI.get()->getScop();
-    else
-      return nullptr;
-  }
+  /// @return If the current region is a valid for a static control part,
+  ///         return the Polly IR representing this static control part,
+  ///         return null otherwise.
+  Scop *getScop() { return S.get(); }
+  const Scop *getScop() const { return S.get(); }
 
   /// @brief Calculate the polyhedral scop information for a given Region.
   bool runOnRegion(Region *R, RGPassManager &RGM) override;
 
-  void releaseMemory() override { SI.reset(); }
+  void releaseMemory() override { S.reset(); }
 
   void print(raw_ostream &O, const Module *M = nullptr) const override;
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 };
 
 } // end namespace polly
 
 namespace llvm {
 class PassRegistry;
 void initializeScopInfoRegionPassPass(llvm::PassRegistry &);
 } // namespace llvm
 
 #endif

polly/trunk/lib/Analysis/ScopInfo.cpp

if (!scop->hasFeasibleRuntimeContext()) {
     ++ScopFound;
     if (scop->getMaxLoopDepth() > 0)
       ++RichScopFound;
   }
 
   emitOptimizationRemarkAnalysis(F->getContext(), DEBUG_TYPE, *F, End, Msg);
 }
 
-void ScopBuilder::clear() { scop.reset(); }
-
 //===----------------------------------------------------------------------===//
 void ScopInfoRegionPass::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addRequired<LoopInfoWrapperPass>();
   AU.addRequired<RegionInfoPass>();
   AU.addRequired<DominatorTreeWrapperPass>();
   AU.addRequiredTransitive<ScalarEvolutionWrapperPass>();
   AU.addRequiredTransitive<ScopDetection>();
   AU.addRequired<AAResultsWrapperPass>();
bool ScopInfoRegionPass::runOnRegion(Region *R, RGPassManager &RGM) {
   Function *F = R->getEntry()->getParent();
   auto &SE = getAnalysis<ScalarEvolutionWrapperPass>().getSE();
   auto &LI = getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
   auto &AA = getAnalysis<AAResultsWrapperPass>().getAAResults();
   auto const &DL = F->getParent()->getDataLayout();
   auto &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
   auto &AC = getAnalysis<AssumptionCacheTracker>().getAssumptionCache(*F);
 
-  SI.reset(new ScopBuilder(R, AC, AA, DL, DT, LI, SD, SE));
+  ScopBuilder SB(R, AC, AA, DL, DT, LI, SD, SE);
+  S = SB.getScop(); // take ownership of scop object
   return false;
 }
 
 void ScopInfoRegionPass::print(raw_ostream &OS, const Module *) const {
-  Scop *scop;
-  if (SI) {
-    if ((scop = SI->getScop())) {
-      scop->print(OS);
-      return;
-    }
-  }
+  if (S)
+    S->print(OS);
+  else
     OS << "Invalid Scop!\n";
 }
 
 char ScopInfoRegionPass::ID = 0;
 
 Pass *polly::createScopInfoRegionPassPass() { return new ScopInfoRegionPass(); }
 
 INITIALIZE_PASS_BEGIN(ScopInfoRegionPass, "polly-scops",
                       "Polly - Create polyhedral description of Scops", false,