Merge pull request #19858 from dlongnecke-cray/dyno-pass-call-destruc…

…tors-0

Turn some subpasses in callDestructors into PassManager passes (#19858)

Turn some subpasses within the `callDestructors` pass into passes
runnable by the new `PassManager`.

The functions turned into passes are:

  - `createIteratorBreakBlocks()`: Lowering, creates blocks to
    cleanup local variables in an iterator at each yield point
  - `fixupDestructors()`: Generate destructor bodies by issuing
    concrete destructor calls for each field
  - `insertAutoDestroyPrimsForLoopExprTemps()`: Fixes a memory
    leak related to bracket loops being interpreted as types?
    Not entirely sure, but a pass meant to fix a leak.

Additionally, adjust the pass managers to iterate over the `Vec`
and `std::vector` types using fixed-length index based iteration
instead of range based for (e.g., `for (int i = 0; ...)` versus
`for (auto x : vec)`. This is because some passes over e.g.,
`CallExpr` will also insert additional calls into the global vector,
which causes iterator invalidation if the vector is resized.

Preemptively adjust the `convertClassTypes` pass so that it does
not take a function pointer argument, and calls `convertToCanonical`
instead. I think in the future `dyno` should probably just emit
canonical class types directly, though.

Reviewed by @mppf. Thanks!

TESTING

- [x] `ALL` on `linux64`, `standard`

Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>

compiler/AST/DecoratedClassType.cpp

@@ -403,25 +403,25 @@ static Type* convertToCanonical(Type* a) {
 }
 
 
-static void convertClassTypes(Type* (*convert)(Type*)) {
+static void convertClassTypes(void) {
 
   forv_Vec(VarSymbol, var, gVarSymbols) {
-    Type* newT = convert(var->type);
+    Type* newT = convertToCanonical(var->type);
     if (newT != var->type) var->type = newT;
   }
 
   forv_Vec(ArgSymbol, arg, gArgSymbols) {
-    Type* newT = convert(arg->type);
+    Type* newT = convertToCanonical(arg->type);
     if (newT != arg->type) arg->type = newT;
   }
 
   forv_Vec(ShadowVarSymbol, sv, gShadowVarSymbols) {
-    Type* newT = convert(sv->type);
+    Type* newT = convertToCanonical(sv->type);
     if (newT != sv->type) sv->type = newT;
   }
 
   forv_Vec(TypeSymbol, ts, gTypeSymbols) {
-    Type* newT = convert(ts->type);
+    Type* newT = convertToCanonical(ts->type);
     if (newT != ts->type) {
       TypeSymbol* newTS = newT->symbol;
       for_SymbolSymExprs(se, ts) {
@@ -433,7 +433,7 @@ static void convertClassTypes(Type* (*convert)(Type*)) {
     for (size_t i = 0; i < n; i++) {
       NameAndSymbol& ns = ts->type->substitutionsPostResolve[i];
       if (TypeSymbol* ets = toTypeSymbol(ns.value)) {
-        Type* newT = convert(ets->type);
+        Type* newT = convertToCanonical(ets->type);
         if (newT != ets->type) {
           TypeSymbol* newTS = newT->symbol;
           ns.value = newTS;
@@ -443,11 +443,11 @@ static void convertClassTypes(Type* (*convert)(Type*)) {
   }
 
   forv_Vec(FnSymbol, fn, gFnSymbols) {
-    Type* newRetT = convert(fn->retType);
+    Type* newRetT = convertToCanonical(fn->retType);
     if (newRetT != fn->retType) fn->retType = newRetT;
 
     if (fn->iteratorInfo) {
-      Type* newYieldT = convert(fn->iteratorInfo->yieldedType);
+      Type* newYieldT = convertToCanonical(fn->iteratorInfo->yieldedType);
       if (newYieldT != fn->iteratorInfo->yieldedType)
         fn->iteratorInfo->yieldedType = newYieldT;
     }
@@ -456,7 +456,7 @@ static void convertClassTypes(Type* (*convert)(Type*)) {
     for (size_t i = 0; i < n; i++) {
       NameAndSymbol& ns = fn->substitutionsPostResolve[i];
       if (TypeSymbol* ets = toTypeSymbol(ns.value)) {
-        Type* newT = convert(ets->type);
+        Type* newT = convertToCanonical(ets->type);
         if (newT != ets->type) {
           TypeSymbol* newTS = newT->symbol;
           ns.value = newTS;
@@ -471,7 +471,7 @@ void convertClassTypesToCanonical() {
 
   // Anything that has unmanaged pointer type should be using the canonical
   // type instead.
-  convertClassTypes(convertToCanonical);
+  convertClassTypes();
 
   // At this point the TypeSymbols for the unmanaged types should
   // be removed from the tree. Using these would be an error.

compiler/AST/iterator.cpp

@@ -1186,7 +1186,6 @@ buildZip4(IteratorInfo* ii, std::vector<BaseAST*>& asts, BlockStmt* singleLoop)
   ii->zip4->body->replace(zip4body);
 }
 
-
 //
 // Handle IBB - Iterator Break Block.
 //
@@ -1211,23 +1210,34 @@ buildZip4(IteratorInfo* ii, std::vector<BaseAST*>& asts, BlockStmt* singleLoop)
 // pretend to throw an error into the forall's error handler. Mimicking
 // the control flow is sufficient for that.
 //
-void createIteratorBreakBlocks() {
-  for_alive_in_Vec(CallExpr, yield, gCallExprs)
-    if (yield->isPrimitive(PRIM_YIELD))
-      if (FnSymbol* parent = toFnSymbol(yield->parentSymbol)) {
-        SET_LINENO(yield);
-        BlockStmt* ibbBody = new BlockStmt();
-        if (ForallStmt* fs = enclosingForallStmt(yield)) {
-          // An empty IBB is: if gIteratorBreakToken then throw;
-          ibbBody->insertAtTail(gotoForallErrorHandler(fs));
-        } else {
-          // An empty IBB is: if gIteratorBreakToken then return;
-          Symbol* epLab = parent->getOrCreateEpilogueLabel();
-          ibbBody->insertAtTail(new GotoStmt(GOTO_RETURN, epLab));
-        }
-        yield->insertAfter(new CondStmt(new SymExpr(gIteratorBreakToken),
-                                        ibbBody));
-      }
+bool CreateIteratorBreakBlocks::shouldProcess(CallExpr* call) {
+  if (call->inTree() && call->isPrimitive(PRIM_YIELD)) {
+    if (isFnSymbol(call->parentSymbol)) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+void CreateIteratorBreakBlocks::process(CallExpr* call) {
+  INT_ASSERT(call->isPrimitive(PRIM_YIELD));
+  FnSymbol* parent = toFnSymbol(call->parentSymbol);
+  INT_ASSERT(parent);
+  CallExpr* yield = call;
+
+  SET_LINENO(yield);
+  BlockStmt* ibbBody = new BlockStmt();
+  if (ForallStmt* fs = enclosingForallStmt(yield)) {
+    // An empty IBB is: if gIteratorBreakToken then throw;
+    ibbBody->insertAtTail(gotoForallErrorHandler(fs));
+  } else {
+    // An empty IBB is: if gIteratorBreakToken then return;
+    Symbol* epLab = parent->getOrCreateEpilogueLabel();
+    ibbBody->insertAtTail(new GotoStmt(GOTO_RETURN, epLab));
+  }
+  yield->insertAfter(new CondStmt(new SymExpr(gIteratorBreakToken),
+                                  ibbBody));
 }
 
 // Find and return the IBB for the given yield stmt.

compiler/include/PassManager.h

@@ -24,6 +24,7 @@
 #include <memory>
 #include <unordered_set>
 #include <vector>
+#include "vec.h"
 
 //
 // A PassT is a Pass that runs on things of type T and can return results of ResultType
@@ -178,8 +179,46 @@ template <typename T> using PassTList = std::vector<std::unique_ptr<PassT<T>>>;
 // today so that eg. a pass over CallExpr is actually a Function pass
 // with an adapter to visit every CallExpr without a performance hit.
 //
+// When working over a "std::vector" or "Vec" type, the PassManager will
+// only iterate over elements present when the pass started running, by
+// noting the size at that time.
+//
 class PassManager {
  private:
+  // Run pass over many and return it's results (if any). Specialization
+  // for vector with fixed length iteration in order to avoid iterator
+  // invalidation.
+  template <typename T, typename R, typename Elt>
+  R runPassImpl(PassT<T, R>& pass, const std::vector<Elt>& xs) {
+    const int stop = xs.size();
+    for (int i = 0; i < stop; i++) {
+      pass.run(xs[i]);
+    }
+
+    while (pass.hasNext()) {
+      pass.processNext();
+    }
+
+    return pass.getResult();
+  }
+
+  // Run pass over many and return it's results (if any). Specialization
+  // for vector with fixed length iteration in order to avoid iterator
+  // invalidation.
+  template <typename T, typename R, typename Elt>
+  R runPassImpl(PassT<T, R>& pass, const Vec<Elt>& xs) {
+    const int stop = xs.size();
+    for (int i = 0; i < stop; i++) {
+      pass.run(xs.v[i]);
+    }
+
+    while (pass.hasNext()) {
+      pass.processNext();
+    }
+
+    return pass.getResult();
+  }
+
   // Run pass over many and return it's results (if any)
   template <typename T, typename R, typename Container>
   R runPassImpl(PassT<T, R>& pass, const Container& xs) {

compiler/include/passes.h

@@ -219,6 +219,35 @@ class LocalizeGlobals : public PassT<FnSymbol*> {
   std::vector<SymExpr*> symExprs;
 };
 
+/**
+  Inserts blocks into iterator bodies after each yield which are
+  responsible for handling cleanup actions at that yield point.
+*/
+class CreateIteratorBreakBlocks : public PassT<CallExpr*> {
+ public:
+  bool shouldProcess(CallExpr* call) override;
+  void process(CallExpr* call) override;
+};
+
+/**
+  Builds up destructor bodies by inserting destructor calls for fields.
+*/
+class FixupDestructors : public PassT<FnSymbol*> {
+ public:
+  bool shouldProcess(FnSymbol* fn) override;
+  void process(FnSymbol* fn) override;
+};
+
+/**
+  This pass over calls is a workaround which plugs leaks coming from forall
+  exprs that are array types.
+*/
+class AutoDestroyLoopExprTemps: public PassT<CallExpr*> {
+ public:
+  bool shouldProcess(CallExpr* call) override;
+  void process(CallExpr* call) override;
+};
+
 class BulkCopyRecords : public PassT<FnSymbol*> {
  public:
   bool shouldProcess(FnSymbol* fn) override;

compiler/include/vec.h

@@ -111,7 +111,7 @@ class Vec {
   C first();
   C *set_in_internal(C a);
   void set_expand();
-  int index(C a);
+  int index(C a) const;
   void set_to_vec();
   void vec_to_set();
   void move(Vec<C,S> &v);
@@ -125,8 +125,8 @@ class Vec {
   C* begin() const { return v; }
   C* end() const { return v + n; }
   Vec<C,S>& operator=(Vec<C,S> &v) { this->copy(v); return *this; }
-  int length () { return n; }
-  int size() { return n; }
+  int length () const { return n; }
+  int size() const { return n; }
 
  private:
   void move_internal(Vec<C,S> &v);
@@ -378,7 +378,7 @@ Vec<C,S>::first() {
 }
 
 template <class C, int S> inline int
-Vec<C,S>::index(C a) {
+Vec<C,S>::index(C a) const {
   for (C *c = v; c < v + n; c++)
     if (*c == a)
       return c - v;