[PATCH] [OPENMP] OMPExecutableDirective re-factoring

[Alexander Musman]

  Hal,

  I've removed whitespace changes (will commit them in a separate patch).
  I hope this change makes OMPExecutableDirective more readable, because it becomes more clear that these arrays are tail-allocated (as they are calculated when needed) when one looks at the class definition.

  This patch is actually my implementation for the following idea suggested by Richard Smith in one of previous reviews:
  «Something to address in a future patch: it seems pointless for `OMPExecutableDirective` to have tail-allocated operands and to store `ArrayRef`s pointing to them. Either you should only store the number of each kind of operand and dynamically compute the locations of those arrays, or allocate them separately (the block allocator has no memory overhead, so this is exactly equivalent apart from marginally reducing space wasted to fragmentation).»

  Thanks,
  Alexander

Hi rsmith, doug.gregor, ABataev, gribozavr, hfinkel, cbergstrom, fraggamuffin,

http://llvm-reviews.chandlerc.com/D2977

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D2977?vs=7585&id=8157#toc

Files:
  include/clang/AST/StmtOpenMP.h
  lib/AST/Stmt.cpp

Index: include/clang/AST/StmtOpenMP.h
===================================================================
--- include/clang/AST/StmtOpenMP.h
+++ include/clang/AST/StmtOpenMP.h
@@ -38,10 +38,22 @@
   SourceLocation StartLoc;
   /// \brief Ending location of the directive.
   SourceLocation EndLoc;
-  /// \brief Pointer to the list of clauses.
-  llvm::MutableArrayRef<OMPClause *> Clauses;
-  /// \brief Associated statement (if any) and expressions.
-  llvm::MutableArrayRef<Stmt *> StmtAndExpressions;
+  /// \brief Numbers of clauses.
+  const unsigned NumClauses;
+  /// \brief Number of child expressions/stmts.
+  const unsigned NumChildren;
+  /// \brief Offset from this to the start of clauses.
+  /// There are NumClauses pointers to clauses, they are followed by
+  /// NumChildren pointers to child stmts/exprs (if the directive type
+  /// requires an associated stmt, then it has to be the first of them).
+  const unsigned ClausesOffset;
+
+  /// \brief Get the clauses storage.
+  llvm::MutableArrayRef<OMPClause *> getClauses() {
+    OMPClause **ClauseStorage = reinterpret_cast<OMPClause **>(
+        reinterpret_cast<char *>(this) + ClausesOffset);
+    return llvm::MutableArrayRef<OMPClause *>(ClauseStorage, NumClauses);
+  }
 
 protected:
   /// \brief Build instance of directive of class \a K.
@@ -54,15 +66,11 @@
   template <typename T>
   OMPExecutableDirective(const T *, StmtClass SC, OpenMPDirectiveKind K,
                          SourceLocation StartLoc, SourceLocation EndLoc,
-                         unsigned NumClauses, unsigned NumberOfExpressions)
+                         unsigned NumClauses, unsigned NumChildren)
       : Stmt(SC), Kind(K), StartLoc(StartLoc), EndLoc(EndLoc),
-        Clauses(reinterpret_cast<OMPClause **>(
-                    reinterpret_cast<char *>(this) +
-                    llvm::RoundUpToAlignment(sizeof(T),
-                                             llvm::alignOf<OMPClause *>())),
-                NumClauses),
-        StmtAndExpressions(reinterpret_cast<Stmt **>(Clauses.end()),
-                           NumberOfExpressions) {}
+        NumClauses(NumClauses), NumChildren(NumChildren),
+        ClausesOffset(llvm::RoundUpToAlignment(sizeof(T),
+                                               llvm::alignOf<OMPClause *>())) {}
 
   /// \brief Sets the list of variables for this clause.
   ///
@@ -74,7 +82,7 @@
   ///
   /// /param S Associated statement.
   ///
-  void setAssociatedStmt(Stmt *S) { StmtAndExpressions[0] = S; }
+  void setAssociatedStmt(Stmt *S) { *child_begin() = S; }
 
 public:
   /// \brief Returns starting location of directive kind.
@@ -94,19 +102,16 @@
   void setLocEnd(SourceLocation Loc) { EndLoc = Loc; }
 
   /// \brief Get number of clauses.
-  unsigned getNumClauses() const { return Clauses.size(); }
+  unsigned getNumClauses() const { return NumClauses; }
 
   /// \brief Returns specified clause.
   ///
   /// \param i Number of clause.
   ///
-  OMPClause *getClause(unsigned i) const {
-    assert(i < Clauses.size() && "index out of bound!");
-    return Clauses[i];
-  }
+  OMPClause *getClause(unsigned i) const { return clauses()[i]; }
 
   /// \brief Returns statement associated with the directive.
-  Stmt *getAssociatedStmt() const { return StmtAndExpressions[0]; }
+  Stmt *getAssociatedStmt() const { return const_cast<Stmt *>(*child_begin()); }
 
   OpenMPDirectiveKind getDirectiveKind() const { return Kind; }
 
@@ -116,12 +121,19 @@
   }
 
   child_range children() {
-    return child_range(StmtAndExpressions.begin(), StmtAndExpressions.end());
+    Stmt **ChildStorage = reinterpret_cast<Stmt **>(getClauses().end());
+    return child_range(ChildStorage, ChildStorage + NumChildren);
   }
 
-  ArrayRef<OMPClause *> clauses() { return Clauses; }
+  ArrayRef<OMPClause *> clauses() {
+    return ArrayRef<OMPClause *>(getClauses().begin(), NumClauses);
+  }
 
-  ArrayRef<OMPClause *> clauses() const { return Clauses; }
+  ArrayRef<OMPClause *> clauses() const {
+    OMPClause *const *ClauseStorage = reinterpret_cast<OMPClause *const *>(
+        reinterpret_cast<const char *>(this) + ClausesOffset);
+    return ArrayRef<OMPClause *>(ClauseStorage, NumClauses);
+  }
 };
 
 /// \brief This represents '#pragma omp parallel' directive.
Index: lib/AST/Stmt.cpp
===================================================================
--- lib/AST/Stmt.cpp
+++ lib/AST/Stmt.cpp
@@ -1193,9 +1193,9 @@
 }
 
 void OMPExecutableDirective::setClauses(ArrayRef<OMPClause *> Clauses) {
-  assert(Clauses.size() == this->Clauses.size() &&
+  assert(Clauses.size() == getNumClauses() &&
          "Number of clauses is not the same as the preallocated buffer");
-  std::copy(Clauses.begin(), Clauses.end(), this->Clauses.begin());
+  std::copy(Clauses.begin(), Clauses.end(), getClauses().begin());
 }
 
 OMPParallelDirective *OMPParallelDirective::Create(
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2977.2.patch
Type: text/x-patch
Size: 4890 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140327/851f3633/attachment.bin>