Merge pull request #14691 from mppf/expiring-deinit

Adjust point of deinitialization based on expiring value analysis

This PR updates the compiler to use expiring value analysis in order to
choose the point at which `deinit` for a local record variable is run. It
does this according to the discussion in #13704.

This PR includes several minor modification to the rules described in
#13704.
1. Variables referred to in `begin` blocks are considered to expire at
   the end of the function (and issue #14750 discusses this and how it
   should interact with`sync` blocks)
2. An exception for `=` and `init=` for managed types. Because ownership
   transfer from non-nil is one of the use cases, we need the analysis to
   not consider `x` to potentially alias `y` in the below code, when both
   are `owned` - `var x = y;`. In general, if these are classes or
   records, they could share a reference to a field. For `owned` however,
   the right-hand-side of the initialization is left `nil` after the
   statement, and so there is no potential for aliasing between `x` and
   `y` after the statement. Additionally, for `shared`, while `x` and `y`
   refer to the same memory, this has no impact on when `x` or `y` can be
   deinitialized or a copy could be avoided. Therefore, when considering
   if a potential alias is created, the analysis ignores assignment and
   copy-initialization of managed types.

Additionally, it improves the split-init implementation. Previous to this
PR, an initialization point like `x = functionReturningByValue()` would
cause the compiler to emit `init=` when it did not need to. Additionally,
the compiler was emitting `deinit` calls for variables that were declared
but not yet initialized if there was a return from the block. Lastly,
this PR extends split-init to work with arrays (where before it was
disabled in resolution).

Lastly, it adjusts the lifetime checker to now require a lifetime clause
on `=` or `<=>` overloads for types that contain borrows.

Implementation steps:
* Remove FnSymbol::replaceReturnSymbol - it is unused
* rename `_statementLevelSymbol` to `chpl_statementLevelSymbol` and add
  `astr_chpl_statementLevelSymbol` to the compiler
* Print out variables stored directly in ForallStmts as "other variables"
  in the AST logs
* Add flags to disable split init and early deinit; change
  fLifetimeChecking to fNoLifetimeChecking to reflect the current default
* Move addAutoDestroyCalls to after ret-arg transform so it can run after
  lifetime checking (where the expiring value analysis runs) and make
  several adjustments to handle ret-args in addAutoDestroyCalls.
* Add code to normalize to mark the end-of-statement. Normalize adds a
  `PRIM_END_OF_STATEMENT` call after the end of each statement that isn't
  already at the end of a block or that refers to user variables.
  `PRIM_END_OF_STATEMENT` arguments are SymExprs referring to user
  variables that should be preserved until the end of that statement,
  which matters for code that is removed before callDestructors, such as
  `x.type`. Adjusted the unordered forall optimization and flatten
  functions to ignore these `PRIM_END_OF_STATEMENT` calls. These
  `PRIM_END_OF_STATEMENT` calls are removed in callDestructors.
* Updated the normalizer code for split-init to properly handle `x =
  makeRecord()` in addition to `x = new MyRecord()` and to store the
  type-expr in a temporary that is passed to both
  `PRIM_INIT_VAR_SPLIT_DECL` and `PRIM_INIT_VAR_SPLIT_INIT` to enable split
  init of arrays
* Updated the normalizer code for split init to explicitly handle
  TryStmts even though they currently disable split-init
* Updated functionResolution to handle split-init of arrays
* addAutoDestroyCalls creates a map from a statement to which variables
  should be destroyed there in order to implement the early deinit. Any
  variables not in the map will be destroyed according to previous rules
  (i.e. at the end of the block). Once deinit calls for these are added,
  the variable is added to the ignore set.
* updated addAutoDestroyCalls and AutoDestroyScope to track which
  variables have been initialized (in case they are split-inited) and to
  propagate that information to parent blocks (in case a variable is
  initialized with split-init in a nested block).

Test updates:
 * updating tests that check deinit occurs for end-of-block variables on
   return/throw/etc to force some variables to be end-of-block
 * work-arounds for #14746
 * updating tests for new deinit behavior and in some cases test both
   last-mention and end-of-block
 * new tests of new behavior

Reviewed by @vasslitvinov - thanks!

- [x] full local testing

Future Work:
 * document deinit points in the language specification -
   Cray/chapel-private#690
 * deinitialize in reverse initialization order rather than reverse
   declaration order - Cray/chapel-private#691

Related Design Questions:
 * Expiring value analysis interaction with `=` and `init=` for managed
   types (described above)
 * Lifetime checking and = overloads #14757 
 * Split-init interactions with deinitialization order #14747 
 * Should split-init be allowed within a try block? #14748 
 * Should last mention deinitialization point go inside blocks? #14749 
 * Expiring value analysis and begin blocks #14750 
 * Split-init interaction with warning about local-from-distributed
   #14746
 * Should unused record variables be removed? #14758

compiler/AST/AstDump.cpp

@@ -481,6 +481,14 @@ bool AstDump::enterForallStmt(ForallStmt* node) {
   }
   --mIndent;
   newline();
+  write("other variables");
+  ++mIndent;
+  if (node->fRecIterIRdef) node->fRecIterIRdef->accept(this);
+  if (node->fRecIterICdef) node->fRecIterICdef->accept(this);
+  if (node->fRecIterGetIterator) node->fRecIterGetIterator->accept(this);
+  if (node->fRecIterFreeIterator) node->fRecIterFreeIterator->accept(this);
+  newline();
+  --mIndent;
   write("forall body");
   node->loopBody()->accept(this);
   --mIndent;

compiler/AST/FnSymbol.cpp

@@ -507,36 +507,6 @@ Symbol* FnSymbol::getReturnSymbol() {
   return retval;
 }
 
-
-// Replace the return symbol with 'newRetSymbol',
-// return the previous return symbol.
-// If newRetType != NULL, also update fn->retType.
-Symbol* FnSymbol::replaceReturnSymbol(Symbol* newRetSymbol, Type* newRetType) {
-  CallExpr* ret    = toCallExpr(this->body->body.last());
-  Symbol*   retval = NULL;
-
-  if (ret != NULL && ret->isPrimitive(PRIM_RETURN) == true) {
-    if (SymExpr* sym = toSymExpr(ret->get(1))) {
-      Symbol* prevRetSymbol = sym->symbol();
-
-      sym->setSymbol(newRetSymbol);
-
-      this->retSymbol = newRetSymbol;
-
-      if (newRetType != NULL) {
-        this->retType = newRetType;
-      }
-
-      retval = prevRetSymbol;
-    }
-
-  } else {
-    INT_FATAL(this, "function is not normal");
-  }
-
-  return retval;
-}
-
 // Removes all statements from body and adds all statements from block.
 void FnSymbol::replaceBodyStmtsWithStmts(BlockStmt* block) {
   for_alist(stmt, this->body->body) {

compiler/AST/baseAST.cpp

@@ -163,6 +163,19 @@ void printStatistics(const char* pass) {
   last_nasts = nasts;
 }
 
+/* Certain AST elements, such as PRIM_END_OF_STATEMENT, should just
+   be adjusted when variables are removed. */
+static void remove_weak_links(VarSymbol* var) {
+  if (var != NULL) {
+    for_SymbolSymExprs(se, var) {
+      if (isAlive(se))
+        if (CallExpr* call = toCallExpr(se->parentExpr))
+          if (call->isPrimitive(PRIM_END_OF_STATEMENT))
+            se->remove();
+    }
+  }
+}
+
 // for debugging purposes only
 void trace_remove(BaseAST* ast, char flag) {
   // crash if deletedIdHandle is not initialized but deletedIdFilename is
@@ -187,6 +200,8 @@ void trace_remove(BaseAST* ast, char flag) {
     if (isAlive(ast) || isRootModuleWithType(ast, type)) { \
       g##type##s.v[i##type++] = ast;            \
     } else {                                    \
+      if (E_##type == E_VarSymbol)              \
+        remove_weak_links(toVarSymbol(ast));    \
       trace_remove(ast, 'x');                   \
       delete ast; ast = 0;                      \
     }                                           \
@@ -396,18 +411,23 @@ bool BaseAST::isRefOrWideRef() {
 }
 
 FnSymbol* BaseAST::getFunction() {
-  if (ModuleSymbol* x = toModuleSymbol(this))
-    return x->initFn;
-  else if (FnSymbol* x = toFnSymbol(this))
-    return x;
-  else if (Type* x = toType(this))
-    return x->symbol->getFunction();
-  else if (Symbol* x = toSymbol(this))
-    return x->defPoint->getFunction();
-  else if (Expr* x = toExpr(this))
-    return x->parentSymbol->getFunction();
-  else
-    INT_FATAL(this, "Unexpected case in BaseAST::getFunction()");
+  BaseAST* cur = this;
+  while (cur != NULL) {
+    // base cases
+    if (ModuleSymbol* x = toModuleSymbol(cur))
+      return x->initFn;
+    else if (FnSymbol* x = toFnSymbol(cur))
+      return x;
+    // inductive cases
+    else if (Type* x = toType(cur))
+      cur = x->symbol;
+    else if (Symbol* x = toSymbol(cur))
+      cur = x->defPoint;
+    else if (Expr* x = toExpr(cur))
+      cur = x->parentSymbol;
+    else
+      INT_FATAL(this, "Unexpected case in BaseAST::getFunction()");
+  }
   return NULL;
 }
 

compiler/AST/expr.cpp

@@ -566,7 +566,12 @@ void SymExpr::verify() {
     INT_FATAL(this, "SymExpr::verify %12d: var is NULL", id);
 
   if (var->defPoint) {
-    if (!var->defPoint->inTree())
+    bool isEndOfStatement = false;
+    if (CallExpr* call = toCallExpr(parentExpr))
+      if (call->isPrimitive(PRIM_END_OF_STATEMENT))
+        isEndOfStatement = true;
+
+    if (!var->defPoint->inTree() && !isEndOfStatement)
       INT_FATAL(this, "SymExpr::verify %12d:  var->defPoint is not in AST", id);
   } else {
     if (var != rootModule)

compiler/AST/primitive.cpp

@@ -1063,6 +1063,13 @@ initPrimitive() {
   prim_def(PRIM_TO_NON_NILABLE_CLASS, "to non nilable class", returnInfoToNonNilable, false, false);
 
   prim_def(PRIM_NEEDS_AUTO_DESTROY, "needs auto destroy", returnInfoBool, false, false);
+
+  // Indicates the end of a statement. This is important for the
+  // deinitialization location for some variables.
+  // Any arguments are SymExprs to user variables that should be alive until
+  // after this primitive.
+  prim_def(PRIM_END_OF_STATEMENT, "end of statement", returnInfoVoid, false, false);
+
   prim_def(PRIM_AUTO_DESTROY_RUNTIME_TYPE, "auto destroy runtime type", returnInfoVoid, false, false);
 
   // Accepts 3 arguments:

compiler/AST/symbol.cpp

@@ -1974,6 +1974,7 @@ const char* astrThis = NULL;
 const char* astr_chpl_cname = NULL;
 const char* astr_chpl_forward_tgt = NULL;
 const char* astr_chpl_manager = NULL;
+const char* astr_chpl_statementLevelSymbol = NULL;
 const char* astr_forallexpr = NULL;
 const char* astr_forexpr = NULL;
 const char* astr_loopexpr_iter = NULL;
@@ -2001,6 +2002,7 @@ void initAstrConsts() {
   astr_chpl_cname = astr("_chpl_cname");
   astr_chpl_forward_tgt = astr("_chpl_forward_tgt");
   astr_chpl_manager = astr("_chpl_manager");
+  astr_chpl_statementLevelSymbol = astr("chpl_statementLevelSymbol");
 
   astr_forallexpr    = astr("chpl__forallexpr");
   astr_forexpr       = astr("chpl__forexpr");

compiler/include/AstVisitor.h

@@ -64,6 +64,17 @@ class AstVisitor
                  AstVisitor();
   virtual       ~AstVisitor();
 
+  // Generally an AST visitor has one or two routine per type of AST node.
+  // For nodes that can contain other nodes, it has 2 routines:
+  //   bool enterSomething(Something* node)
+  //   void exitSomething(Something* node)
+  // And for nodes that do not contain other nodes, it has
+  //   void visitSomething(Something* node)
+  //
+  // If enterSomething returns `true`, it indicates that:
+  //   * nested nodes should be visited
+  //   * exitSomething should be called for that node
+
   //
   // The first implementation of this pattern used a traditional naming
   // convention that relied on overloading of virtual functions.  Although

compiler/include/FnSymbol.h

@@ -137,9 +137,9 @@ class FnSymbol : public Symbol {
   LabelSymbol*               getEpilogueLabel();
   LabelSymbol*               getOrCreateEpilogueLabel();
 
+  // getReturnSymbol returns the variable marked RVV, but if
+  // the return-by-ref transformation has been applied, it returns gVoid.
   Symbol*                    getReturnSymbol();
-  Symbol*                    replaceReturnSymbol(Symbol* newRetSymbol,
-                                                 Type*   newRetType);
 
   // Removes all statements from body and adds all statements from block.
   void                       replaceBodyStmtsWithStmts(BlockStmt* block);

compiler/include/driver.h

@@ -150,7 +150,9 @@ extern bool fRegionVectorizer;
 extern bool fGenIDS;
 extern bool fLocal;
 extern bool fIgnoreLocalClasses;
-extern bool fLifetimeChecking;
+extern bool fNoLifetimeChecking;
+extern bool fNoSplitInit;
+extern bool fNoEarlyDeinit;
 extern bool fCompileTimeNilChecking;
 extern bool fOverrideChecking;
 extern int  ffloatOpt;

compiler/include/primitive_list.h

@@ -262,7 +262,9 @@
 
   PRIMITIVE_R(PRIM_IS_POD)
   PRIMITIVE_R(PRIM_NEEDS_AUTO_DESTROY)
+  PRIMITIVE_R(PRIM_END_OF_STATEMENT)
   PRIMITIVE_R(PRIM_AUTO_DESTROY_RUNTIME_TYPE)
+
   PRIMITIVE_R(PRIM_GET_RUNTIME_TYPE_FIELD)
 
   PRIMITIVE_R(PRIM_COERCE)

compiler/include/symbol.h

@@ -695,6 +695,7 @@ extern const char* astrThis;
 extern const char* astr_chpl_cname;
 extern const char* astr_chpl_forward_tgt;
 extern const char* astr_chpl_manager;
+extern const char* astr_chpl_statementLevelSymbol;
 extern const char* astr_forallexpr;
 extern const char* astr_forexpr;
 extern const char* astr_loopexpr_iter;

compiler/main/driver.cpp

@@ -190,7 +190,9 @@ int fLinkStyle = LS_DEFAULT; // use backend compiler's default
 bool fUserSetLocal = false;
 bool fLocal;   // initialized in postLocal()
 bool fIgnoreLocalClasses = false;
-bool fLifetimeChecking = true;
+bool fNoLifetimeChecking = false;
+bool fNoSplitInit = false;
+bool fNoEarlyDeinit = false;
 bool fCompileTimeNilChecking = true;
 bool fOverrideChecking = true;
 bool fieeefloat = false;
@@ -1069,7 +1071,9 @@ static ArgumentDescription arg_desc[] = {
  {"denormalize", ' ', NULL, "Enable [disable] denormalization", "N", &fDenormalize, "CHPL_DENORMALIZE", NULL},
  DRIVER_ARG_DEBUGGERS,
  {"interprocedural-alias-analysis", ' ', NULL, "Enable [disable] interprocedural alias analysis", "n", &fNoInterproceduralAliasAnalysis, NULL, NULL},
- {"lifetime-checking", ' ', NULL, "Enable [disable] lifetime checking pass", "N", &fLifetimeChecking, NULL, NULL},
+ {"lifetime-checking", ' ', NULL, "Enable [disable] lifetime checking pass", "n", &fNoLifetimeChecking, NULL, NULL},
+ {"split-initialization", ' ', NULL, "Enable [disable] support for split initialization", "n", &fNoSplitInit, NULL, NULL},
+ {"early-deinit", ' ', NULL, "Enable [disable] support for early deinit based upon expiring value analysis", "n", &fNoEarlyDeinit, NULL, NULL},
  {"legacy-classes", ' ', NULL, "Deprecated flag - does not affect compilation", "N", &fLegacyClasses, NULL, &warnUponLegacyClasses},
  {"ignore-nilability-errors", ' ', NULL, "Allow compilation to continue by coercing away nilability", "N", &fIgnoreNilabilityErrors, NULL, NULL},
  {"overload-sets-checks", ' ', NULL, "Report potentially hijacked calls", "N", &fOverloadSetsChecks, NULL, NULL},

compiler/optimizations/optimizeForallUnorderedOps.cpp

@@ -128,6 +128,11 @@ static void helpGetLastStmts(Expr* last, std::vector<Expr*>& stmts) {
       if (fn->hasFlag(FLAG_COMPILER_ADDED_REMOTE_FENCE))
         last = last->prev;
 
+  // Ignore calls to PRIM_END_OF_STATEMENT
+  if (CallExpr* call = toCallExpr(last))
+    if (call->isPrimitive(PRIM_END_OF_STATEMENT))
+      last = last->prev;
+
   // Otherwise, add what we got.
   stmts.push_back(last);
 }

compiler/passes/flattenFunctions.cpp

@@ -336,10 +336,11 @@ replaceVarUsesWithFormals(FnSymbol* fn, SymbolMap* vars) {
                    call->isPrimitive(PRIM_ASSIGN)     ||
                    call->isPrimitive(PRIM_SET_MEMBER) )
                   && call->get(1) == se)                                   ||
-                (call->isPrimitive(PRIM_GET_MEMBER))                       ||
-                (call->isPrimitive(PRIM_GET_MEMBER_VALUE))                 ||
-                (call->isPrimitive(PRIM_WIDE_GET_LOCALE))                  ||
-                (call->isPrimitive(PRIM_WIDE_GET_NODE))                    ||
+                call->isPrimitive(PRIM_GET_MEMBER)                         ||
+                call->isPrimitive(PRIM_GET_MEMBER_VALUE)                   ||
+                call->isPrimitive(PRIM_WIDE_GET_LOCALE)                    ||
+                call->isPrimitive(PRIM_WIDE_GET_NODE)                      ||
+                call->isPrimitive(PRIM_END_OF_STATEMENT)                   ||
                 canPassToFn) {
               se->setSymbol(arg); // do not dereference argument in these cases