[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.

include/polly/ScopInfo.h

private:
   /// The introduction of different cases necessarily complicates the memory
   /// access function, but cases that can be statically proven to not happen
   /// will be eliminated later on.
   __isl_give isl_map *foldAccess(const IRAccess &Access,
                                  __isl_take isl_map *AccessRelation,
                                  ScopStmt *Statement);
 
 public:
+  // @brief Common constructor to set the most basic properties.
+  MemoryAccess(AccessType AccType, Instruction *AccInst, ScopStmt *Statement);
+
+  // @brief Setup the object using data of an IRAccess.
+  void init(const IRAccess &Access, const ScopArrayInfo *SAI, int Identifier);
+
   /// @brief Create a memory access from an access in LLVM-IR.
   ///
   /// @param Access     The memory access.
   /// @param AccInst    The access instruction.
   /// @param Statement  The statement that contains the access.
   /// @param SAI        The ScopArrayInfo object for this base pointer.
   /// @param Identifier An identifier that is unique for all memory accesses
   ///                   belonging to the same scop statement.
-  MemoryAccess(const IRAccess &Access, Instruction *AccInst,
-               ScopStmt *Statement, const ScopArrayInfo *SAI, int Identifier);
+  static MemoryAccess *create(const IRAccess &Access, Instruction *AccInst,
+                              ScopStmt *Statement, const ScopArrayInfo *SAI,
+                              int Identifier);
 
   ~MemoryAccess();
 
   /// @brief Get the type of a memory access.
   enum AccessType getType() { return AccType; }
 
   /// @brief Is this a reduction like access?
   bool isReductionLike() const { return RedType != RT_NONE; }

 class ScopStmt {
 public:
   /// @brief List to hold all (scalar) memory accesses mapped to an instruction.
   using MemoryAccessList = std::forward_list<MemoryAccess>;
 
   ScopStmt(const ScopStmt &) = delete;
   const ScopStmt &operator=(const ScopStmt &) = delete;
 
-  /// Create the ScopStmt from a BasicBlock.
-  ScopStmt(Scop &parent, TempScop &tempScop, const Region &CurRegion,
-           BasicBlock &bb, SmallVectorImpl<Loop *> &NestLoops);
-
-  /// Create an overapproximating ScopStmt for the region @p R.
-  ScopStmt(Scop &parent, TempScop &tempScop, const Region &CurRegion, Region &R,
-           SmallVectorImpl<Loop *> &NestLoops);
+  /// @brief Common constructor for all ScopStmts to set the fundamental
+  /// properties.
+  ScopStmt(Scop &parent, ArrayRef<Loop *> OuterLoops);
+
+  /// @brief Initialize this ScopStmt using an overapproximating TempScop
+  /// object.
+  void initFromRegion(TempScop &tempScop, const Region &CurRegion, Region &R);
+
+  /// @brief Create an overapproximating ScopStmt for the region @p R.
+  static ScopStmt *createFromRegion(Scop &parent, TempScop &tempScop,
+                                    const Region &CurRegion, Region &R,
+                                    ArrayRef<Loop *> OuterLoops);
+
+  /// @brief Initialize this ScopStmt using a TempScop object.
+  void initFromBasicBlock(TempScop &tempScop, const Region &CurRegion,
+                          BasicBlock &BB);
+
+  /// @brief Create the ScopStmt from a BasicBlock.
+  static ScopStmt *createFromBasicBlock(Scop &parent, TempScop &tempScop,
+                                        const Region &CurRegion, BasicBlock &BB,
+                                        ArrayRef<Loop *> OuterLoops);
 
 private:
   /// Polyhedral description
   //@{
 
   /// The Scop containing this ScopStmt
   Scop &Parent;
 
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.

lib/Analysis/ScopInfo.cpp

for (int i = Size - 2; i >= 0; --i) {
     MapTwo = isl_map_upper_bound_si(MapTwo, isl_dim_in, i + 1, -1);
 
     MapOne = isl_map_union(MapOne, MapTwo);
     AccessRelation = isl_map_apply_range(AccessRelation, MapOne);
   }
   return AccessRelation;
 }
 
-MemoryAccess::MemoryAccess(const IRAccess &Access, Instruction *AccInst,
-                           ScopStmt *Statement, const ScopArrayInfo *SAI,
-                           int Identifier)
-    : AccType(getMemoryAccessType(Access)), Statement(Statement), Inst(AccInst),
-      newAccessRelation(nullptr) {
+MemoryAccess::MemoryAccess(AccessType AccType, Instruction *AccInst,
+                           ScopStmt *Statement)
+    : AccType(AccType), Statement(Statement), Inst(AccInst),
+      newAccessRelation(nullptr) {}
 
+void MemoryAccess::init(const IRAccess &Access, const ScopArrayInfo *SAI,
+                        int Identifier) {
   isl_ctx *Ctx = Statement->getIslCtx();
   BaseAddr = Access.getBase();
   BaseName = getIslCompatibleName("MemRef_", getBaseAddr(), "");
 
   isl_id *BaseAddrId = SAI->getBasePtrId();
 
   auto IdName = "__polly_array_ref_ " + std::to_string(Identifier);
   Id = isl_id_alloc(Ctx, IdName.c_str(), nullptr);
void MemoryAccess::init(const IRAccess &Access, const ScopArrayInfo *SAI,
   AccessRelation =
       isl_map_set_tuple_id(AccessRelation, isl_dim_out, BaseAddrId);
 
   assumeNoOutOfBound(Access);
   AccessRelation = isl_map_gist_domain(AccessRelation, Statement->getDomain());
   isl_space_free(Space);
 }
 
+MemoryAccess *MemoryAccess::create(const IRAccess &Access, Instruction *AccInst,
+                                   ScopStmt *Statement,
+                                   const ScopArrayInfo *SAI, int Identifier) {
+  auto MA = new MemoryAccess(getMemoryAccessType(Access), AccInst, Statement);
+  MA->init(Access, SAI, Identifier);
+  return MA;
+}
+
 void MemoryAccess::realignParams() {
   isl_space *ParamSpace = Statement->getParent()->getParamSpace();
   AccessRelation = isl_map_align_params(AccessRelation, ParamSpace);
 }
 
 const std::string MemoryAccess::getReductionOperatorStr() const {
   return MemoryAccess::getReductionOperatorStr(getReductionType());
 }
const ScopArrayInfo *SAI = getParent()->getOrCreateScopArrayInfo(
         Access.getBase(), ElementType, Access.Sizes);
 
     if (isApproximated && Access.isWrite())
       Access.setMayWrite();
 
     MemoryAccessList *&MAL = InstructionToAccess[AccessInst];
     if (!MAL)
       MAL = new MemoryAccessList();
-    MAL->emplace_front(Access, AccessInst, this, SAI, MemAccs.size());
-    MemAccs.push_back(&MAL->front());
+    MAL->emplace_front(getMemoryAccessType(Access), AccessInst, this);
+    auto &MA = MAL->front();
+    MA.init(Access, SAI, MemAccs.size());
+    MemAccs.push_back(&MA);
   }
 }
 
 void ScopStmt::realignParams() {
   for (MemoryAccess *MA : *this)
     MA->realignParams();
 
   Domain = isl_set_align_params(Domain, Parent.getParamSpace());

 }
 
 void ScopStmt::deriveAssumptions(BasicBlock *Block) {
   for (Instruction &Inst : *Block)
     if (auto *GEP = dyn_cast<GetElementPtrInst>(&Inst))
       deriveAssumptionsFromGEP(GEP);
 }
 
-ScopStmt::ScopStmt(Scop &parent, TempScop &tempScop, const Region &CurRegion,
-                   Region &R, SmallVectorImpl<Loop *> &Nest)
-    : Parent(parent), BB(nullptr), R(&R), Build(nullptr),
-      NestLoops(Nest.size()) {
-  // Setup the induction variables.
-  for (unsigned i = 0, e = Nest.size(); i < e; ++i)
-    NestLoops[i] = Nest[i];
+ScopStmt::ScopStmt(Scop &parent, ArrayRef<Loop *> OuterLoops)
+    : Parent(parent), NestLoops(OuterLoops.begin(), OuterLoops.end()),
+      BB(nullptr), R(nullptr), Domain(nullptr), Build(nullptr) {}
+
+void ScopStmt::initFromRegion(TempScop &tempScop, const Region &CurRegion,
+                              Region &R) {
+  assert(!this->R && !this->BB);
 
+  this->R = &R;
   BaseName = getIslCompatibleName("Stmt_", R.getNameStr(), "");
 
   Domain = buildDomain(tempScop, CurRegion);
 
   BasicBlock *EntryBB = R.getEntry();
   for (BasicBlock *Block : R.blocks()) {
     buildAccesses(tempScop, Block, Block != EntryBB);
     deriveAssumptions(Block);
   }
   checkForReductions();
 }
 
-ScopStmt::ScopStmt(Scop &parent, TempScop &tempScop, const Region &CurRegion,
-                   BasicBlock &bb, SmallVectorImpl<Loop *> &Nest)
-    : Parent(parent), BB(&bb), R(nullptr), Build(nullptr),
-      NestLoops(Nest.size()) {
-  // Setup the induction variables.
-  for (unsigned i = 0, e = Nest.size(); i < e; ++i)
-    NestLoops[i] = Nest[i];
+static ScopStmt *createFromRegion(Scop &parent, TempScop &tempScop,
+                                  const Region &CurRegion, Region &R,
+                                  ArrayRef<Loop *> OuterLoops) {
+  auto SS = new ScopStmt(parent, OuterLoops);
+  SS->initFromRegion(tempScop, CurRegion, R);
+  return SS;
+}
+
+void ScopStmt::initFromBasicBlock(TempScop &tempScop, const Region &CurRegion,
+                                  BasicBlock &BB) {
+  assert(!this->R && !this->BB);
 
-  BaseName = getIslCompatibleName("Stmt_", &bb, "");
+  this->BB = &BB;
+  BaseName = getIslCompatibleName("Stmt_", &BB, "");
 
   Domain = buildDomain(tempScop, CurRegion);
-  buildAccesses(tempScop, BB);
-  deriveAssumptions(BB);
+  buildAccesses(tempScop, &BB);
+  deriveAssumptions(&BB);
   checkForReductions();
 }
 
+ScopStmt *ScopStmt::createFromBasicBlock(Scop &parent, TempScop &tempScop,
+                                         const Region &CurRegion,
+                                         BasicBlock &BB,
+                                         ArrayRef<Loop *> OuterLoops) {
+  auto SS = new ScopStmt(parent, OuterLoops);
+  SS->initFromBasicBlock(tempScop, CurRegion, BB);
+  return SS;
+}
+
 /// @brief Collect loads which might form a reduction chain with @p StoreMA
 ///
 /// Check if the stored value for @p StoreMA is a binary operator with one or
 /// two loads as operands. If the binary operand is commutative & associative,
 /// used only once (by @p StoreMA) and its load operands are also used only
 /// once, we have found a possible reduction chain. It starts at an operand
 /// load and includes the binary operator and @p StoreMA.
 ///
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)) {
mapToDimension(__isl_take isl_union_set *Domain, int N) {
 
   isl_union_set_free(Domain);
   return isl_multi_union_pw_aff_from_union_pw_multi_aff(Data.Res);
 }
 
 ScopStmt *Scop::addScopStmt(BasicBlock *BB, Region *R, TempScop &tempScop,
                             const Region &CurRegion,
                             SmallVectorImpl<Loop *> &NestLoops) {
-  ScopStmt *Stmt;
+
+  Stmts.emplace_back(*this, NestLoops);
+  ScopStmt *Stmt = &Stmts.back();
   if (BB) {
-    Stmts.emplace_back(*this, tempScop, CurRegion, *BB, NestLoops);
-    Stmt = &Stmts.back();
+    Stmt->initFromBasicBlock(tempScop, CurRegion, *BB);
     StmtMap[BB] = Stmt;
   } else {
     assert(R && "Either basic block or a region expected.");
-    Stmts.emplace_back(*this, tempScop, CurRegion, *R, NestLoops);
-    Stmt = &Stmts.back();
+    Stmt->initFromRegion(tempScop, CurRegion, *R);
     for (BasicBlock *BB : R->blocks())
       StmtMap[BB] = Stmt;
   }
   return Stmt;
 }
 
 __isl_give isl_schedule *Scop::buildScop(TempScop &tempScop,
                                          const Region &CurRegion,
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;

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();