Bug 1183400 - Fold |if| nodes by kind. r=efaust
authorJeff Walden <jwalden@mit.edu>
Wed, 08 Jul 2015 13:54:14 -0700
changeset 287214 7dbbc282b2e75110b67d7e33903a6855c7440075
parent 287213 58129dccd5d8393be998eada7621dfdcdbae27bc
child 287215 5b94aec178f136387abf2da1952b17daff112478
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust
bugs1183400
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1183400 - Fold |if| nodes by kind. r=efaust
js/src/frontend/FoldConstants.cpp
js/src/tests/ecma_6/Statements/if-constant-folding.js
--- a/js/src/frontend/FoldConstants.cpp
+++ b/js/src/frontend/FoldConstants.cpp
@@ -936,36 +936,138 @@ FoldConditional(ExclusiveContext* cx, Pa
         ReplaceNode(nodePtr, replacement);
 
         parser.freeTree(discarded);
     } while (nextNode);
 
     return true;
 }
 
+static bool
+FoldIf(ExclusiveContext* cx, ParseNode** nodePtr, Parser<FullParseHandler>& parser,
+       bool inGenexpLambda)
+{
+    ParseNode** nextNode = nodePtr;
+
+    do {
+        // |nextNode| on entry points to the initial |if| to be folded.  Reset
+        // it to exit the loop when the |else| arm isn't another |if|.
+        nodePtr = nextNode;
+        nextNode = nullptr;
+
+        ParseNode* node = *nodePtr;
+        MOZ_ASSERT(node->isKind(PNK_IF));
+        MOZ_ASSERT(node->isArity(PN_TERNARY));
+
+        ParseNode*& expr = node->pn_kid1;
+        if (!Fold(cx, &expr, parser, inGenexpLambda, SyntacticContext::Condition))
+            return false;
+
+        ParseNode*& consequent = node->pn_kid2;
+        if (!Fold(cx, &consequent, parser, inGenexpLambda, SyntacticContext::Other))
+            return false;
+
+        ParseNode*& alternative = node->pn_kid3;
+        if (alternative) {
+            // If in |if (C) T; else F;| we have |F| as another |if|,
+            // *iteratively* constant-fold |F| *after* folding |C| and |T| (and
+            // possibly completely replacing the whole thing with |T| or |F|);
+            // otherwise fold F normally.  Making |nextNode| non-null causes
+            // this loop to run again to fold F.
+            if (alternative->isKind(PNK_IF)) {
+                nextNode = &alternative;
+            } else {
+                if (!Fold(cx, &alternative, parser, inGenexpLambda,
+                          SyntacticContext::Other))
+                {
+                    return false;
+                }
+            }
+        }
+
+        // Eliminate the consequent or alternative if the condition has
+        // constant truthiness.  Don't eliminate if we have an |if (0)| in
+        // trailing position in a generator expression, as this is a special
+        // form we can't fold away.
+        Truthiness t = Boolish(expr);
+        if (t == Unknown || inGenexpLambda)
+            continue;
+
+        // Careful!  Either of these can be null: |replacement| in |if (0) T;|,
+        // and |discarded| in |if (true) T;|.
+        ParseNode* replacement;
+        ParseNode* discarded;
+        if (t == Truthy) {
+            replacement = consequent;
+            discarded = alternative;
+        } else {
+            replacement = alternative;
+            discarded = consequent;
+        }
+
+        bool performReplacement = true;
+        if (discarded) {
+            // A declaration that hoists outside the discarded arm prevents the
+            // |if| from being folded away.
+            bool containsHoistedDecls;
+            if (!ContainsHoistedDeclaration(cx, discarded, &containsHoistedDecls))
+                return false;
+
+            performReplacement = !containsHoistedDecls;
+        }
+
+        if (!performReplacement)
+            continue;
+
+        if (!replacement) {
+            // If there's no replacement node, we have a constantly-false |if|
+            // with no |else|.  Replace the entire thing with an empty
+            // statement list.
+            parser.prepareNodeForMutation(node);
+            node->setKind(PNK_STATEMENTLIST);
+            node->setArity(PN_LIST);
+            node->makeEmpty();
+        } else {
+            // As with PNK_CONDITIONAL, replace only if the replacement isn't a
+            // definition.  As there, the rationale for this restriction is
+            // unclear and undocumented: tolerated only because a failure to
+            // optimize *should* be safe.  The best guess is that this test was
+            // an incomplete, buggy version of the |ContainsHoistedDeclaration|
+            // test.
+            if (replacement->isDefn())
+                continue;
+
+            // Replacement invalidates |nextNode|, so reset it (if the
+            // replacement requires folding) or clear it (if |alternative|
+            // is dead code) as needed.
+            if (nextNode)
+                nextNode = (*nextNode == replacement) ? nodePtr : nullptr;
+            ReplaceNode(nodePtr, replacement);
+
+            // Morph the original node into a discardable node, then
+            // aggressively free it and the discarded arm (if any) to suss out
+            // any bugs in the preceding logic.
+            node->setKind(PNK_STATEMENTLIST);
+            node->setArity(PN_LIST);
+            node->makeEmpty();
+            if (discarded)
+                node->append(discarded);
+            parser.freeTree(node);
+        }
+    } while (nextNode);
+
+    return true;
+}
+
 bool
 Fold(ExclusiveContext* cx, ParseNode** pnp, Parser<FullParseHandler>& parser, bool inGenexpLambda,
      SyntacticContext sc)
 {
     JS_CHECK_RECURSION(cx, return false);
 
-    ParseNode** restartNode = nullptr;
-    SyntacticContext restartContext;
-
-    bool mightHaveHoistedDeclarations = true;
-
-    if (false) {
-      restart:
-        if (!restartNode)
-            return true;
-        pnp = restartNode;
-        sc = restartContext;
-        restartNode = nullptr;
-    }
-
     ParseNode* pn = *pnp;
     ParseNode* pn1 = nullptr;
     ParseNode* pn2 = nullptr;
     ParseNode* pn3 = nullptr;
 
     switch (pn->getKind()) {
       case PNK_NEWTARGET:
       case PNK_NOP:
@@ -1016,16 +1118,19 @@ Fold(ExclusiveContext* cx, ParseNode** p
 
       case PNK_DELETEPROP:
       case PNK_DELETESUPERPROP:
         return FoldDeleteProperty(cx, pn, parser, inGenexpLambda);
 
       case PNK_CONDITIONAL:
         return FoldConditional(cx, pnp, parser, inGenexpLambda);
 
+      case PNK_IF:
+        return FoldIf(cx, pnp, parser, inGenexpLambda);
+
       case PNK_NOT:
         return FoldNot(cx, pn, parser, inGenexpLambda);
 
       case PNK_BITNOT:
       case PNK_POS:
       case PNK_NEG:
         return FoldUnaryArithmetic(cx, pn, parser, inGenexpLambda);
 
@@ -1074,17 +1179,16 @@ Fold(ExclusiveContext* cx, ParseNode** p
       case PNK_RETURN:
       case PNK_IMPORT:
       case PNK_EXPORT_FROM:
       case PNK_EXPORT_DEFAULT:
       case PNK_FORIN:
       case PNK_FOROF:
       case PNK_FORHEAD:
       case PNK_CLASS:
-      case PNK_IF:
       case PNK_TRY:
       case PNK_OR:
       case PNK_AND:
       case PNK_BITOR:
       case PNK_BITXOR:
       case PNK_BITAND:
       case PNK_STRICTEQ:
       case PNK_EQ:
@@ -1178,43 +1282,40 @@ Fold(ExclusiveContext* cx, ParseNode** p
 
         // Save the list head in pn1 for later use.
         pn1 = pn->pn_head;
         pn2 = nullptr;
         break;
       }
 
       case PN_TERNARY:
+        MOZ_ASSERT(!pn->isKind(PNK_CONDITIONAL),
+                   "should be skipping this above");
+        MOZ_ASSERT(!pn->isKind(PNK_IF),
+                   "should be skipping this above");
         /* Any kid may be null (e.g. for (;;)). */
         if (pn->pn_kid1) {
-            if (!Fold(cx, &pn->pn_kid1, parser, inGenexpLambda, condIf(pn, PNK_IF)))
+            if (!Fold(cx, &pn->pn_kid1, parser, inGenexpLambda, SyntacticContext::Other))
                 return false;
         }
         pn1 = pn->pn_kid1;
 
         if (pn->pn_kid2) {
             if (!Fold(cx, &pn->pn_kid2, parser, inGenexpLambda, condIf(pn, PNK_FORHEAD)))
                 return false;
             if (pn->isKind(PNK_FORHEAD) && pn->pn_kid2->isKind(PNK_TRUE)) {
                 parser.freeTree(pn->pn_kid2);
                 pn->pn_kid2 = nullptr;
             }
         }
         pn2 = pn->pn_kid2;
 
         if (pn->pn_kid3) {
-            MOZ_ASSERT(!pn->isKind(PNK_CONDITIONAL),
-                       "should be skipping this above");
-            if (pn->isKind(PNK_IF)) {
-                restartNode = &pn->pn_kid3;
-                restartContext = SyntacticContext::Other;
-            } else {
-                if (!Fold(cx, &pn->pn_kid3, parser, inGenexpLambda, SyntacticContext::Other))
-                    return false;
-            }
+            if (!Fold(cx, &pn->pn_kid3, parser, inGenexpLambda, SyntacticContext::Other))
+                return false;
         }
         pn3 = pn->pn_kid3;
         break;
 
       case PN_BINARY:
       case PN_BINARY_OBJ:
         if (pn->isKind(PNK_OR) || pn->isKind(PNK_AND)) {
             // Propagate Condition context through logical connectives.
@@ -1274,90 +1375,19 @@ Fold(ExclusiveContext* cx, ParseNode** p
 
     // The immediate child of a PNK_DELETE* node should not be replaced
     // with node indicating a different syntactic form; |delete x| is not
     // the same as |delete (true && x)|. See bug 888002.
     //
     // pn is the immediate child in question. Its descendants were already
     // constant-folded above, so we're done.
     if (sc == SyntacticContext::Delete)
-        goto restart;
+        return true;
 
     switch (pn->getKind()) {
-      case PNK_IF:
-        if (mightHaveHoistedDeclarations) {
-            bool result;
-            if (ParseNode* consequent = pn2) {
-                if (!ContainsHoistedDeclaration(cx, consequent, &result))
-                    return false;
-                if (result)
-                    break;
-            }
-            if (ParseNode* alternative = pn3) {
-                if (!ContainsHoistedDeclaration(cx, alternative, &result))
-                    return false;
-                if (result)
-                    break;
-            }
-        }
-        mightHaveHoistedDeclarations = false;
-
-        /* Reduce 'if (C)  T; else F' into T for true C, F for false. */
-        switch (pn1->getKind()) {
-          case PNK_NUMBER:
-            if (pn1->pn_dval == 0 || IsNaN(pn1->pn_dval))
-                pn2 = pn3;
-            break;
-          case PNK_STRING:
-            if (pn1->pn_atom->length() == 0)
-                pn2 = pn3;
-            break;
-          case PNK_TRUE:
-            break;
-          case PNK_FALSE:
-          case PNK_NULL:
-            pn2 = pn3;
-            break;
-          default:
-            /* Early return to dodge common code that copies pn2 to pn. */
-            goto restart;
-        }
-
-#if JS_HAS_GENERATOR_EXPRS
-        /* Don't fold a trailing |if (0)| in a generator expression. */
-        if (!pn2 && inGenexpLambda)
-            break;
-#endif
-
-        if (pn2 && !pn2->isDefn()) {
-            if (restartNode && *restartNode == pn2)
-                restartNode = pnp;
-            ReplaceNode(pnp, pn2);
-            pn = pn2;
-        }
-        if (!pn2 || (pn->isKind(PNK_SEMI) && !pn->pn_kid)) {
-            /*
-             * False condition and no else, or an empty then-statement was
-             * moved up over pn.  Either way, make pn an empty block (not an
-             * empty statement, which does not decompile, even when labeled).
-             * NB: pn must be a PNK_IF as PNK_CONDITIONAL can never have a null
-             * kid or an empty statement for a child.
-             */
-            parser.prepareNodeForMutation(pn);
-            pn->setKind(PNK_STATEMENTLIST);
-            pn->setArity(PN_LIST);
-            pn->makeEmpty();
-        }
-        if (pn3 && pn3 != pn2) {
-            if (restartNode && *restartNode == pn3)
-                restartNode = nullptr;
-            parser.freeTree(pn3);
-        }
-        break;
-
       case PNK_OR:
       case PNK_AND:
         if (sc == SyntacticContext::Condition) {
             ParseNode** listp = &pn->pn_head;
             MOZ_ASSERT(*listp == pn1);
             uint32_t orig = pn->pn_count;
             do {
                 Truthiness t = Boolish(pn1);
@@ -1536,16 +1566,17 @@ Fold(ExclusiveContext* cx, ParseNode** p
       case PNK_TYPEOFNAME:
       case PNK_TYPEOFEXPR:
       case PNK_VOID:
       case PNK_NOT:
       case PNK_BITNOT:
       case PNK_POS:
       case PNK_NEG:
       case PNK_CONDITIONAL:
+      case PNK_IF:
         MOZ_CRASH("should have been fully handled above");
 
       case PNK_ELEM: {
         // An indexed expression, pn1[pn2]. A few cases can be improved.
         PropertyName* name = nullptr;
         if (pn2->isKind(PNK_STRING)) {
             JSAtom* atom = pn2->pn_atom;
             uint32_t index;
@@ -1620,17 +1651,17 @@ Fold(ExclusiveContext* cx, ParseNode** p
             } else {
                 pn->setKind(PNK_FALSE);
                 pn->setOp(JSOP_FALSE);
             }
             pn->setArity(PN_NULLARY);
         }
     }
 
-    goto restart;
+    return true;
 }
 
 bool
 frontend::FoldConstants(ExclusiveContext* cx, ParseNode** pnp, Parser<FullParseHandler>* parser)
 {
     // Don't fold constants if the code has requested "use asm" as
     // constant-folding will misrepresent the source text for the purpose
     // of type checking. (Also guard against entering a function containing
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_6/Statements/if-constant-folding.js
@@ -0,0 +1,35 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+//-----------------------------------------------------------------------------
+var gTestfile = "if-constant-folding.js";
+var BUGNUMBER = 1183400;
+var summary =
+  "Don't crash constant-folding an |if| governed by a truthy constant, whose " +
+  "alternative statement is another |if|";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+// Perform |if| constant folding correctly when the condition is constantly
+// truthy and the alternative statement is another |if|.
+if (true)
+{
+  assertEq(true, true, "sanity");
+}
+else if (42)
+{
+  assertEq(false, true, "not reached");
+  assertEq(true, false, "also not reached");
+}
+
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");