[Polly] Move computations out of constructors

[Meinersbur]

Summary

It is common practice to keep constructors lightweight. The reasons include:

• The vtable during the constructor's execution is set to the static type of the object, not to the vtable of the derived class. That is, method calls behave differently in constructors and ordinary methods. This way it is possible to call unimplemented methods of abstract classes, which usually results in a segmentation fault.

• If an exception is thrown in the constructor, the destructor is not called, potentially leaking memory.

• Code in constructors cannot be called in a regular way, e.g. from non-constructor methods of derived classes.

• Because it is common practice, people (including me) do not expect the constructor to do more than initializing data and skip them when looking for bugs.

Not all of these are applicable to LLVM (e.g. exceptions are disabled).

This patch refactors out the computational work in the constructors of Scop, ScopStmt and IslAst into regular init functions and introduces static create-functions as replacement. The constructor ScopArrayInfo also has calls, but it is small and does not call methods of its own class.

[grosser]

Hi Michael,

I think this looks good to me. Just a minor thing. Could you possibly make the constructors
and init() functions private such that only the create*() functions are part of the public interface
and any user is always sure to get a fully initialized class.

Best,
Tobias

[Meinersbur]

I did where I could (IslAst)

ScopStmt and MemoryAccess ctors are used in emplace_back/emplace_font. I can change these only if I change the std::vector to store pointers to the objects (or dirty tricks like creating at a different place and the move it into the vector).

Actually, changing these vectors may be a good idea - so the objects do not need to be movable anymore. What do you think?

[Meinersbur]

Note: they are not stored in std::vector, but std::deque and std::forward_list, otherwise we'd have a problem if these objects are being relocated.

[jdoerfert]

I'm not sure about the whole "moving code out of constructors" part [1] but I am sure about two things:

1. It is not worth it to introduce things one would call "dirty tricks".
2. Introducing explicit memory managment or pointers to objects that are allocated at different places is always error prone. Our forward_lists and vectors with emplaced objects might not be the best solution but at least easy to reason about wrt. memory.

[1] I never experience any of the claimed problems hence I do not see the need for a change. However, if the new code/interface is at least as clean as the current one I do not see a problem either.

[grosser]

Hi Michael,

just a vector of ScopStmt pointers is what we had before and we moved away of this for memory safety reasons (see discussion '[polly] r238090 - Use unique_ptr to clarify ownership of ScopStmt' on llvm-commits). I agree with Johannes, that we should not add new hacks to move out computations from the constructor and also don't think that having both public factor methods and public (partial) constructors is better than what we have now.However, I also have currently a good suggestion of how to do it better.

The islAst part seems non-controversial. Feel free to commit it if you like. Also, maybe add some comments into the header.

polly/trunk/include/polly/ScopInfo.h

private:
   /// In a program with int and float pointers annotated with tbaa information
   /// we would probably generate two alias groups, one for the int pointers and
   /// one for the float pointers.
   ///
   /// During code generation we will create a runtime alias check for each alias
   /// group to ensure the SCoP is executed in an alias free environment.
   MinMaxVectorPairVectorTy MinMaxAliasGroups;
 
+  /// @brief Scop constructor; used by static createFromTempScop
+  Scop(Region &R, ScalarEvolution &SE, isl_ctx *ctx, unsigned MaxLoopDepth);
+
+  /// @brief Initialize this ScopInfo using a TempScop object.
+  void initFromTempScop(TempScop &TempScop, LoopInfo &LI, ScopDetection &SD);
+
   /// Create the static control part with a region, max loop depth of this
   /// region and parameters used in this region.
-  Scop(TempScop &TempScop, LoopInfo &LI, ScalarEvolution &SE, ScopDetection &SD,
+  static Scop *createFromTempScop(TempScop &TempScop, LoopInfo &LI,
+                                  ScalarEvolution &SE, ScopDetection &SD,
                                   isl_ctx *ctx);
 
   /// @brief Check if a basic block is trivial.
   ///
   /// A trivial basic block does not contain any useful calculation. Therefore,
   /// it does not need to be represented as a polyhedral statement.
   ///
   /// @param BB The basic block to check
   /// @param tempScop TempScop returning further information regarding the Scop.

polly/trunk/lib/Analysis/ScopInfo.cpp

if (MaxLD == 0)
     return 1;
 
   assert(MinLD >= 1 && "Minimal loop depth should be at least one");
   assert(MaxLD >= MinLD &&
          "Maximal loop depth was smaller than mininaml loop depth?");
   return MaxLD - MinLD + 1;
 }
 
-Scop::Scop(TempScop &tempScop, LoopInfo &LI, ScalarEvolution &ScalarEvolution,
-           ScopDetection &SD, isl_ctx *Context)
-    : SE(&ScalarEvolution), R(tempScop.getMaxRegion()), IsOptimized(false),
-      MaxLoopDepth(getMaxLoopDepthInRegion(tempScop.getMaxRegion(), LI, SD)) {
-  IslCtx = Context;
+Scop::Scop(Region &R, ScalarEvolution &ScalarEvolution, isl_ctx *Context,
+           unsigned MaxLoopDepth)
+    : SE(&ScalarEvolution), R(R), IsOptimized(false),
+      MaxLoopDepth(MaxLoopDepth), IslCtx(Context) {}
 
+void Scop::initFromTempScop(TempScop &TempScop, LoopInfo &LI,
+                            ScopDetection &SD) {
   buildContext();
 
   SmallVector<Loop *, 8> NestLoops;
 
   // Build the iteration domain, access functions and schedule functions
   // traversing the region tree.
-  Schedule = buildScop(tempScop, getRegion(), NestLoops, LI, SD);
+  Schedule = buildScop(TempScop, getRegion(), NestLoops, LI, SD);
   if (!Schedule)
     Schedule = isl_schedule_empty(getParamSpace());
 
   realignParams();
   addParameterBounds();
   simplifyAssumedContext();
 
   assert(NestLoops.empty() && "NestLoops not empty at top level!");
 }
 
+Scop *Scop::createFromTempScop(TempScop &TempScop, LoopInfo &LI,
+                               ScalarEvolution &SE, ScopDetection &SD,
+                               isl_ctx *ctx) {
+  auto &R = TempScop.getMaxRegion();
+  auto MaxLoopDepth = getMaxLoopDepthInRegion(R, LI, SD);
+  auto S = new Scop(R, SE, ctx, MaxLoopDepth);
+  S->initFromTempScop(TempScop, LI, SD);
+  return S;
+}
+
 Scop::~Scop() {
   isl_set_free(Context);
   isl_set_free(AssumedContext);
   isl_schedule_free(Schedule);
 
   // Free the alias groups
   for (MinMaxVectorPairTy &MinMaxAccessPair : MinMaxAliasGroups) {
     for (MinMaxAccessTy &MMA : MinMaxAccessPair.first) {
bool ScopInfo::runOnRegion(Region *R, RGPassManager &RGM) {
   TempScop *tempScop = getAnalysis<TempScopInfo>().getTempScop(R);
 
   // This region is no Scop.
   if (!tempScop) {
     scop = nullptr;
     return false;
   }
 
-  scop = new Scop(*tempScop, LI, SE, SD, ctx);
+  scop = Scop::createFromTempScop(*tempScop, LI, SE, SD, ctx);
 
   DEBUG(scop->print(dbgs()));
 
   if (!PollyUseRuntimeAliasChecks) {
     // Statistics.
     ++ScopFound;
     if (scop->getMaxLoopDepth() > 0)
       ++RichScopFound;

polly/trunk/lib/CodeGen/IslAst.cpp

 static cl::opt<bool> NoEarlyExit(
     "polly-no-early-exit",
     cl::desc("Do not exit early if no benefit of the Polly version was found."),
     cl::Hidden, cl::init(false), cl::ZeroOrMore, cl::cat(PollyCategory));
 
 namespace polly {
 class IslAst {
 public:
-  IslAst(Scop *Scop, const Dependences &D);
-
+  static IslAst *create(Scop *Scop, const Dependences &D);
   ~IslAst();
 
   /// Print a source code representation of the program.
   void pprint(llvm::raw_ostream &OS);
 
   __isl_give isl_ast_node *getAst();
 
   /// @brief Get the run-time conditions for the Scop.
   __isl_give isl_ast_expr *getRunCondition();
 
 private:
   Scop *S;
   isl_ast_node *Root;
   isl_ast_expr *RunCondition;
 
+  IslAst(Scop *Scop);
+  void init(const Dependences &D);
+
   void buildRunCondition(__isl_keep isl_ast_build *Build);
 };
 } // End namespace polly.
 
 /// @brief Free an IslAstUserPayload object pointed to by @p Ptr
 static void freeIslAstUserPayload(void *Ptr) {
   delete ((IslAstInfo::IslAstUserPayload *)Ptr);
 }
static bool benefitsFromPolly(Scop *Scop, bool PerformParallelTest) {
   if (!PerformParallelTest && !Scop->isOptimized() &&
       Scop->getAliasGroups().empty())
     return false;
 
   // The default assumption is that Polly improves the code.
   return true;
 }
 
-IslAst::IslAst(Scop *Scop, const Dependences &D)
-    : S(Scop), Root(nullptr), RunCondition(nullptr) {
+IslAst::IslAst(Scop *Scop) : S(Scop), Root(nullptr), RunCondition(nullptr) {}
 
+void IslAst::init(const Dependences &D) {
   bool PerformParallelTest = PollyParallel || DetectParallel ||
                              PollyVectorizerChoice != VECTORIZER_NONE;
 
   // Skip AST and code generation if there was no benefit achieved.
-  if (!benefitsFromPolly(Scop, PerformParallelTest))
+  if (!benefitsFromPolly(S, PerformParallelTest))
     return;
 
   isl_ctx *Ctx = S->getIslCtx();
   isl_options_set_ast_build_atomic_upper_bound(Ctx, true);
   isl_ast_build *Build;
   AstBuildUserInfo BuildInfo;
 
   if (UseContext)
void IslAst::init(const Dependences &D) {
 
   buildRunCondition(Build);
 
   Root = isl_ast_build_node_from_schedule(Build, S->getScheduleTree());
 
   isl_ast_build_free(Build);
 }
 
+IslAst *IslAst::create(Scop *Scop, const Dependences &D) {
+  auto Ast = new IslAst(Scop);
+  Ast->init(D);
+  return Ast;
+}
+
 IslAst::~IslAst() {
   isl_ast_node_free(Root);
   isl_ast_expr_free(RunCondition);
 }
 
 __isl_give isl_ast_node *IslAst::getAst() { return isl_ast_node_copy(Root); }
 __isl_give isl_ast_expr *IslAst::getRunCondition() {
   return isl_ast_expr_copy(RunCondition);

 bool IslAstInfo::runOnScop(Scop &Scop) {
   if (Ast)
     delete Ast;
 
   S = &Scop;
 
   const Dependences &D = getAnalysis<DependenceInfo>().getDependences();
 
-  Ast = new IslAst(&Scop, D);
+  Ast = IslAst::create(&Scop, D);
 
   DEBUG(printScop(dbgs(), Scop));
   return false;
 }
 
 __isl_give isl_ast_node *IslAstInfo::getAst() const { return Ast->getAst(); }
 __isl_give isl_ast_expr *IslAstInfo::getRunCondition() const {
   return Ast->getRunCondition();