Convert unconverted ForLoops invoking recursive iterators into CForLoops
Convert unconverted ForLoops invoking recursive iterators into CForLoops

Most ForLoops that invoke recursive iterators are expanded through a call to
expandRecursiveIteratorsInline(), but there are two exceptions:
1. When the forLoop is contained in an iterator that is is flagged as
   recursive, and
2. When the forLoop is contained in a task function that is called by a
   recursive iterator.

Previously, these exceptions left ForLoop constructs in the tree.  ForLoop
constructs remaining in the tree were converted downstream by corresponding
clauses in buildZip1() and buildZip3().  But the conversion was nonsensical,
as proven by the fact that valgrind complained about code generated by the one
test case that exercised that code path::

   classes/bradc/arrayInClass/genericArrayInClass-otharrs.chpl

In this commit, the (experimental) solution is to capture the above special
cases (where conversion of the forLoop is not performed) and to pass these
cases instead through the code that converts non-inlined iterators into
CForLoop constructs.  Such ForLoops are already being converted in this way
when the --no-inline-iterators flag is tossed, so letting these cases fall
through to that code is a change that can be made with high confidence.

This change should enable the assertion that all forLoop constructs have been
removed from the tree after all calls to expandForLoop() have completed.  Then,
the clauses for handling ForLoop constructs in buildZip?() and related routines
can be removed.
diff --git a/compiler/resolution/lowerIterators.cpp b/compiler/resolution/lowerIterators.cpp
index e85b708..dd3a947 100644
--- a/compiler/resolution/lowerIterators.cpp
+++ b/compiler/resolution/lowerIterators.cpp
@@ -172,7 +172,7 @@ static void computeRecursiveIteratorSet() {
     FnSymbol* currFn = taskFn;
     while (true) {
       if (currFn->calledBy->n == 0) {
-        // currFn has on callers, so by inference neither does taskFn.
+        // currFn has no callers, so by inference neither does taskFn.
         // That being the case, we can bail -- no harm done.
         break;
       }
@@ -1152,7 +1152,9 @@ expandBodyForIteratorInline(ForLoop*       forLoop,
                             TaskFnCopyMap& taskFnCopies);

 /// \param call A for loop block primitive.
-static void
+static bool
+// Returns true if the given ForLoop was handled (converted and removed from
+// the tree); false otherwise.
 expandIteratorInline(ForLoop* forLoop) {
   Symbol*   ic       = forLoop->iteratorGet()->var;
   FnSymbol* iterator = ic->type->defaultInitializer->getFormal(1)->type->defaultInitializer;
@@ -1160,17 +1162,26 @@ expandIteratorInline(ForLoop* forLoop) {
   if (iterator->hasFlag(FLAG_RECURSIVE_ITERATOR)) {
     // NOAKES 2014/11/30  Only 6 tests, some with minor variations, use this path

-    //
+    // The iterand of the forLoop is marked as a recursive iterator.  This is
+    // the case we are looking for.  But first, we have to weed out a few
+    // troublesome cases.
+
+    // sjd:
     // loops over recursive iterators in recursive iterators only need
     // to be handled in the recursive iterator function
     //
     if (forLoop->parentSymbol->hasFlag(FLAG_RECURSIVE_ITERATOR)) {
-
-    // ditto for task functions called from recursive iterators
+//      printf("ForLoop %d parent %d has FLAG_RECURSIVE_ITERATOR\n", forLoop->id, forLoop->parentSymbol->id);
+      return false;
+    // vass: ditto for task functions called from recursive iterators
     } else if (taskFunInRecursiveIteratorSet.set_in(forLoop->parentSymbol)) {
-
+//      printf("ForLoop %d parent %d is in taskFunInRecursiveIteratorSet\n", forLoop->id, forLoop->parentSymbol->id);
+      return false;
     } else {
+//      printf("ForLoop %d calling %d expanded inline\n", forLoop->id, iterator->id);
       expandRecursiveIteratorInline(forLoop);
+      INT_ASSERT(!forLoop->inTree());
+      return true;
     }

   } else {
@@ -1191,7 +1202,7 @@ expandIteratorInline(ForLoop* forLoop) {
     // if the loop being expanded was order independent, all of the yielding
     // loops in the body are also order independent. Note that this must occur
     // after the ibody replaces the forLoop since findEnclosingLoop() requires
-    // that it's argument be in the AST. It must occur before yields are
+    // that its argument be in the AST. It must occur before yields are
     // replaced in the functions below though.
     if (isOrderIndependent) {
       std::vector<CallExpr*> callExprs;
@@ -1216,6 +1227,10 @@ expandIteratorInline(ForLoop* forLoop) {
     collect_asts(ibody, asts);

     replaceIteratorFormalsWithIteratorFields(iterator, ic, asts);
+
+    // We can return true if forLoop has been removed from the tree.
+    INT_ASSERT(!forLoop->inTree());
+    return true;
   }
 }

@@ -1655,22 +1670,35 @@ static void
 expandForLoop(ForLoop* forLoop) {
   SymExpr*   se2      = forLoop->iteratorGet();
   VarSymbol* iterator = toVarSymbol(se2->var);
+  bool converted;

-  if (!fNoInlineIterators &&
-      iterator->type->defaultInitializer->getFormal(1)->type->defaultInitializer->iteratorInfo &&
-      canInlineIterator(iterator->type->defaultInitializer->getFormal(1)->type->defaultInitializer) &&
-      (iterator->type->dispatchChildren.n == 0 ||
-       (iterator->type->dispatchChildren.n == 1 &&
-        iterator->type->dispatchChildren.v[0] == dtObject))) {
-    expandIteratorInline(forLoop);
-
-  } else if (!fNoInlineIterators && canInlineSingleYieldIterator(iterator) &&
+  if (!fNoInlineIterators)
+  {
+    if (iterator->type->defaultInitializer->getFormal(1)->type->defaultInitializer->iteratorInfo &&
+        canInlineIterator(iterator->type->defaultInitializer->getFormal(1)->type->defaultInitializer) &&
+        (iterator->type->dispatchChildren.n == 0 ||
+         (iterator->type->dispatchChildren.n == 1 &&
+          iterator->type->dispatchChildren.v[0] == dtObject))) {
+      converted = expandIteratorInline(forLoop);
+    }
+    // Conversion fails above if there is a recursive iterator call in a
+    // recursive iterator, or if there is a task function call in a recursive
+    // iterator.  In either case, it seems like a bad idea to try to perform
+    // inlineSingleYieldIterator() on the same forLoop.  That is the reason for
+    // the unequivocal "else".
+    // To try the other olternative, replace the following line with:
+    // if (!converted && canInlineSingleYieldIterator(iterator) &&
+    else if (canInlineSingleYieldIterator(iterator) &&
             (iterator->type->dispatchChildren.n == 0 ||
              (iterator->type->dispatchChildren.n == 1 &&
               iterator->type->dispatchChildren.v[0] == dtObject))) {
-    inlineSingleYieldIterator(forLoop);
+      inlineSingleYieldIterator(forLoop);
+      converted = true;
+    }
+  }

-  } else {
+  if (! converted)
+  {
     // This code handles zippered iterators, dynamic iterators, and any other
     // iterator that cannot be inlined.

CoryMcCartan/chapel,hildeth/chapel,chizarlicious/chapel,chizarlicious/chapel,CoryMcCartan/chapel,hildeth/chapel,chizarlicious/chapel,hildeth/chapel,chizarlicious/chapel,CoryMcCartan/chapel,hildeth/chapel,CoryMcCartan/chapel,CoryMcCartan/chapel,chizarlicious/chapel,CoryMcCartan/chapel,chizarlicious/chapel,hildeth/chapel,chizarlicious/chapel,hildeth/chapel,hildeth/chapel,CoryMcCartan/chapel