Closed Bug 1183400 Opened 5 years ago Closed 5 years ago

Perform constant-folding by examining node kind, not arity-then-kind with "syntactic context" information threaded through all folding

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(34 files, 1 obsolete file)

4.79 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.22 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.58 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.98 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.88 KB, patch
efaust
: review+
Details | Diff | Splinter Review
7.02 KB, patch
efaust
: review+
Details | Diff | Splinter Review
28.20 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.29 KB, patch
efaust
: review+
Details | Diff | Splinter Review
11.70 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.50 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.91 KB, patch
efaust
: review+
Details | Diff | Splinter Review
14.73 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.89 KB, patch
efaust
: review+
Details | Diff | Splinter Review
7.76 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.75 KB, patch
efaust
: review+
Details | Diff | Splinter Review
7.58 KB, patch
efaust
: review+
Details | Diff | Splinter Review
11.82 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.43 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.86 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.72 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.12 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.16 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.26 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.09 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.21 KB, patch
shu
: review+
Details | Diff | Splinter Review
2.32 KB, patch
shu
: review+
Details | Diff | Splinter Review
3.88 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.10 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.18 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.68 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.92 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.87 KB, patch
efaust
: review+
Details | Diff | Splinter Review
26.99 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.19 KB, patch
efaust
: review+
Details | Diff | Splinter Review
Next up in the list.

To be sure, some people would rather nuke this entire component from orbit than modify it, clean it up, etc.  But there are components of this that are indisputably necessary -- converting obj["prop"] to obj.prop, folding together string additions, possibly others although less clearly.  And these changes help out code that doesn't quite make it to Ion.  So let's fix what's there and defer these sorts of bigger arguments to another day.
The current setup now is that we do a first round of folding sub-nodes based upon the node's arity, then we arbitrarily decide to stop there if we're folding in deletion context, then we do a second round of folding based upon the node's kind, then we try to fold constants into true/false inside conditionals.

The desired setup is a single switch based on node arity.

However, getting from here to there is a bit tortuous.  Where possible, I'm going to *try* to add cases to this new kind-switch that *completely* handle the entirety of folding.  Sometimes this has to inline the if-deleting/if-condition bits, exactly as written.  Other times, I end the case with a |goto afterFolding;| that skips the if-delete case and the secondary kind-switch, jumping straight to the condition code.  Things must get worse, sometimes, before they can get better.

But never fear!  By the end of the patchwork here, Fold() contains one switch on kind, every case (or group of them) performs all steps of folding, and the only fallthrough from the switch is to a crash.  And as a bonus, SyntacticContext dies.  (It really only ever was a hack for our being unable to have folding of conditions, and of deletions, be more careful how they folded those sub-components.)

So, buckle in.  I'll try to keep the patch-stream to a handful at a time so it's not overwhelming.
Attachment #8633185 - Flags: review?(efaustbmo)
Some of these are further folded by the after-folding code, so I jump there and skip the by-kind folding that targets none of these.  And as already-simplified nullary nodes, they're not going to change if touched in deletion contexxt, so who cares about that test, either.  :-)
Attachment #8633187 - Flags: review?(efaustbmo)
There are two variants of |typeof|: that on a name, where if the name doesn't exist it's as if it evaluated to |undefined|; and that on an expression, fully evaluated.  Because of the PNK_TYPEOF{NAME,EXPR} split, folding an expression into a name still preserves a PNK_TYPEOFEXPR node, and the emitter treats *EXPR nodes as always having expression semantics.  So we can fold away without worries, for expressions.

Names, on the other hand, don't fold into anything at all: they just stay names.  So for PNK_TYPEOFNAME, just return true straightaway, after a few structural assertions.

The truthiness of typeof isn't known by Boolish, so no need to proceed to afterFolding code.  (If our IR split effects from result, we *could* know the result of typeof was truthy, but we don't do that, so eh.)
Attachment #8633191 - Flags: review?(efaustbmo)
Again, an effects/result split would make this Condition bit better, but oh well.

And to reiterate, by the end of this bug's patches this Condition bit disappears entirely, so there's no reason to common up this condition-folding code in a method or anything.  And the early-return for delete context doesn't matter because nothing treats PNK_VOID specially (even as part of a catch-all) in the arity-switch.
Attachment #8633195 - Flags: review?(efaustbmo)
The also-morph bit of this is a little fruity.  However, if we do this, SyntacticContext::Delete becomes unnecessary, and the fruity is limited to exactly one place.  Seems good enough.  (Actual removal waits awhile, tho.)

You'll also observe that this is pretty amenable to removing PNK_DELETESUPER* whenever you get to that, I hope.  :-)
Attachment #8633203 - Flags: review?(efaustbmo)
Boolean inversion seems rather different from the arithmetic operators, to me -- and needs a different SyntacticContext at present -- so I split it away from the other mathematical unary operations.

The unary arithmetic folding code stops when a number's been reached, without folding (in condition context) to true/false.  There's no reason to go further as the old code did -- Boolish handles numbers just fine.  In this way kind-wise operation makes for doing less stuff.
Attachment #8633209 - Flags: review?(efaustbmo)
When folding ++/--, I want to assert that the target remains valid through the folding.  That requires having a parser instance around.  Lots of drear here, nothing very interesting happening.

Judging from my last skim now, I leave as an exercise for the reader determining when the options parameter became unused here.  :-\
Attachment #8633216 - Flags: review?(efaustbmo)
These are basically just unary, except for the extra assertion.  I'll take the extra assertion over sharing code, myself.
Attachment #8633220 - Flags: review?(efaustbmo)
Okay, this one's the first complicated patch.  I've basically just ported the existing algorithm; I've simply given variables new names and added a bunch more comments explaining what's being done.  I am not going to explain what the code does -- if the comments don't make it clear, then I probably should add more comments, or otherwise make changes here.  Tell me what's unclear, if necessary, and I can fix it.

I also eliminated the trailing if-Condition constant folding.  If the list doesn't fold to one element, then Boolish would return Unknown, so no reason to bother.  But if it *did* return Truthy/Falsy, then all we're left with is a node that's the overall result of the expression.  And if that's in another condition, it'll *again* get folded down as needed, or be consumed as the result of the operation.

The only difference I see is that |if ("foo")| might have been if-true before, and now it'd be if-"foo".  This is most easily fixed at the end of the patches, at which point there's a FoldCondition method that would be the most ideal place to add a final replace-even-truthy-stuff-with-true step.  If it really even matters, generally.  :-)
Attachment #8633228 - Flags: review?(efaustbmo)
Attachment #8633230 - Flags: review?(efaustbmo)
I'll cut it off there and wait for a few of these to be reviewed before posting more.

And actually, I think I was mistaken about the if-"foo" thing above.  With the patches posted applied, I get:

js> dis(); if ("foo") print();
loc     op
-----   --
main:
00000:  getgname "dis"
00005:  gimplicitthis "dis"
00010:  call 0
00013:  setrval
00014:  getgname "print"
00019:  gimplicitthis "print"
00024:  call 0
00027:  setrval
00028:  retrval

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1    14 [  14] xdelta  
  1:    1    14 [   0] colspan 6
  3:    1    14 [   0] colspan 12
  5:    1    28 [  14] xdelta  
  6:    1    28 [   0] colspan 8

That's certainly eliminating the test entirely, so I think there's no even micro-regression in folding of conditions in that bit.
Attachment #8633185 - Flags: review?(efaustbmo) → review+
Comment on attachment 8633187 [details] [diff] [review]
Constant-fold nullary nodes

Review of attachment 8633187 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +627,5 @@
>        case PNK_BREAK:
>        case PNK_CONTINUE:
> +      case PNK_TEMPLATE_STRING:
> +      case PNK_THIS:
> +      case PNK_GENERATOR:

Is there a reason for this reordering? I'm not seeing why this is better than before
Attachment #8633187 - Flags: review?(efaustbmo) → review+
Comment on attachment 8633191 [details] [diff] [review]
Fold typeof nodes by examining their kind

Review of attachment 8633191 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.
Attachment #8633191 - Flags: review?(efaustbmo) → review+
Comment on attachment 8633195 [details] [diff] [review]
Fold |void <expr>| expressions by kind

Review of attachment 8633195 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +652,5 @@
> +    ParseNode*& expr = node->pn_kid;
> +    if (!Fold(cx, &expr, handler, options, inGenexpLambda, SyntacticContext::Other))
> +        return false;
> +
> +    if (sc == SyntacticContext::Condition) {

Good lord this is ugly. I would have thought we could fold this to undefined, and then undefined would be falsey anyways when we evaluated the condition. Am I missing some reason why we need false explicitly?
Attachment #8633195 - Flags: review?(efaustbmo) → review+
Comment on attachment 8633203 [details] [diff] [review]
Fold delete nodes by kind

Review of attachment 8633203 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: js/src/frontend/FoldConstants.cpp
@@ +745,5 @@
> +    if (!Fold(cx, &expr, handler, options, inGenexpLambda, SyntacticContext::Other))
> +        return false;
> +
> +    MOZ_ASSERT(expr->isKind(oldKind),
> +               "kind should have remained invariant under folding");

Cute. Nice assertion.

@@ +800,5 @@
>  
> +      case PNK_DELETENAME: {
> +        MOZ_ASSERT(pn->isArity(PN_UNARY));
> +        MOZ_ASSERT(pn->pn_kid->isKind(PNK_NAME));
> +        return true;

Makes sense.
Attachment #8633203 - Flags: review?(efaustbmo) → review+
Oh stab me with a fork, bug 1172895 did violence to all this.  :-\  It's almost half-tempting to back out that change and just redo it myself atop all these patches, but for now I'm trying to rebase through it.
Rebasing seems to be be doable, if quite painful, but the end result might as well be me rewriting the relevant parts of that patch.  Oh well.  I may go back and change PNK_IF into a proper list node when this is all done, as a better fix for this.

Whole bunch more patches to come.

(In reply to Eric Faust [:efaust] from comment #12)
> Is there a reason for this reordering? I'm not seeing why this is better
> than before

Version control hack to ensure the full list of nullary nodes was in the patch, for reviewing.  Cue gps saying "but MozReview solves this for you".

(In reply to Eric Faust [:efaust] from comment #14)
> Good lord this is ugly. I would have thought we could fold this to
> undefined, and then undefined would be falsey anyways when we evaluated the
> condition. Am I missing some reason why we need false explicitly?

That's basically right, but I'm just preserving the existing code.  Ultimately I do simplify it that way once SyntacticContext dies.
Keywords: leave-open
I'm not sure how important it actually is that ?:?:?:... iteratively fold without consuming linear stack space, but the current code does so.  (Probably because it's impossible to disentangle if/?: and for no other reason.)  Keep doing that.

This plus the next patch to disentangle if-nodes mean all of the changes in bug 1172895 have been extricated into the narrowest by-kind operations, so subsequent patching can be simple and understandable again.  I recommend reviewing this patch and the next one before doing any of the rest.
Attachment #8636360 - Flags: review?(efaustbmo)
Attached patch Fold |if| nodes by kind (obsolete) — Splinter Review
It *almost* feels to me like the isDefn() checks in here are supposed to guard against a function expression morphing into a function statement, but I haven't made it work to get a bug if I disable the don't-replace-if-defn logic.  Not worth caring enough about, IMO, to figure it out 100%.
Attachment #8636361 - Flags: review?(efaustbmo)
Attachment #8636361 - Attachment is obsolete: true
Attachment #8636361 - Flags: review?(efaustbmo)
Comment on attachment 8633209 [details] [diff] [review]
Fold !/~/+/- by kind

Review of attachment 8633209 [details] [diff] [review]:
-----------------------------------------------------------------

Nice refactoring of the logic to shorten it.

::: js/src/frontend/FoldConstants.cpp
@@ +779,5 @@
> +
> +        handler.prepareNodeForMutation(node);
> +        node->setKind(newval ? PNK_TRUE : PNK_FALSE);
> +        node->setArity(PN_NULLARY);
> +        node->setOp(newval ? JSOP_TRUE : JSOP_FALSE);

As an aside, remind me why we are using the JSOp field of PNK_* nodes that represent emitting constants?
Attachment #8633209 - Flags: review?(efaustbmo) → review+
Comment on attachment 8633216 [details] [diff] [review]
Pass a Parser reference to Fold, not separate handler/options

Review of attachment 8633216 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.

::: js/src/frontend/Parser.h
@@ +389,5 @@
>      /* State specific to the kind of parse being performed. */
>      ParseHandler handler;
>  
> +    void prepareNodeForMutation(Node node) { handler.prepareNodeForMutation(node); }
> +    void freeTree(Node node) { handler.freeTree(node); }

This is fine. parser.handler.foo() would have been fine as well.
Attachment #8633216 - Flags: review?(efaustbmo) → review+
Comment on attachment 8633220 [details] [diff] [review]
Fold increment/decrement operations by kind

Review of attachment 8633220 [details] [diff] [review]:
-----------------------------------------------------------------

Yup.
Attachment #8633220 - Flags: review?(efaustbmo) → review+
Attachment #8639658 - Flags: review?(efaustbmo)
This treats most binary operators using one method, but it separates out PNK_POW because it's simpler to handle separately.

The next patch gets rid of FoldBinaryNumeric and eliminates about 35 lines of code, but it's separate for readability.
Attachment #8639662 - Flags: review?(efaustbmo)
Every reader of this file will be glad to see the clarification of the node-munging code into its caller, I think.  :-)
Attachment #8639663 - Flags: review?(efaustbmo)
If the renaming of pn1/pn2/pn3 here doesn't make you cry sweet tears of joy, I don't know what will.
Attachment #8639665 - Flags: review?(efaustbmo)
I grant that this is complicated, but the logic is well-commented, and I think it tracks worlds more cleanly than the old logic did, with its weird mess of statefulness that I still half-believe to be buggy.

And I'll call it for now with this latest batch of reviews.  More to come once a handful of these have been reviewed.
Attachment #8639670 - Flags: review?(efaustbmo)
Comment on attachment 8633228 [details] [diff] [review]
Fold and/or expressions by kind

Review of attachment 8633228 [details] [diff] [review]:
-----------------------------------------------------------------

Great added comments.
Attachment #8633228 - Flags: review?(efaustbmo) → review+
Comment on attachment 8633230 [details] [diff] [review]
Fold function nodes by kind

Review of attachment 8633230 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +937,5 @@
> +        return true;
> +
> +    // Note: pn_body is null for lazily-parsed functions.
> +    if (ParseNode*& functionBody = node->pn_body) {
> +        if (!Fold(cx, &functionBody, parser, node->pn_funbox->inGenexpLambda,

This is cute. That type is a little much, but I kinda like it.
Attachment #8633230 - Flags: review?(efaustbmo) → review+
Comment on attachment 8636360 [details] [diff] [review]
Fold ?: expressions by kind

Review of attachment 8636360 [details] [diff] [review]:
-----------------------------------------------------------------

First documented comment I've seen codifying the fear I feel all the time when removing code. ;)

::: js/src/frontend/FoldConstants.cpp
@@ +774,5 @@
> +        if (!Fold(cx, &expr, handler, options, inGenexpLambda, SyntacticContext::Condition))
> +            return false;
> +
> +        ParseNode*& ifTruthy = node->pn_kid2;
> +        if (!Fold(cx, &ifTruthy, handler, options, inGenexpLambda, SyntacticContext::Other))

I'll just leave a remark here that the patch goes to great length to keep C?T: C2?T2:F chains from consuming linear stack space. It's also possible that we could write C ? C2 ? T : F : F2. This will get recursively folded. If we care, we can file a followup.

@@ +811,5 @@
> +        // a definition.
> +        //
> +        // The rationale for this pre-existing restriction is unclear; if you
> +        // discover it, please document it!  This lack of explanation is
> +        // tolerated only because failing to optimize *should* always be okay.

"_/o\_"

Can we add a comment involving the word "speculatively" that describes the function nonsense we discussed IRL?
Attachment #8636360 - Flags: review?(efaustbmo) → review+
Comment on attachment 8636896 [details] [diff] [review]
Fold |if| nodes by kind, with a one-character =/== typo-fix and a targeted test

Review of attachment 8636896 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +873,5 @@
> +
> +        // 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.

No way that should be PNK_IF, but OK. Greener grass over there.

@@ +878,5 @@
> +        Truthiness t = Boolish(expr);
> +        if (t == Unknown || inGenexpLambda)
> +            continue;
> +
> +        // Careful!  If we have |if (0) ...| with no |else|, the discarded

Perhaps this comment belongs below with the |if (discarded)|?

@@ +895,5 @@
> +        if (discarded) {
> +            // A declaration that hoists outside the arm of the |if| prevents
> +            // the |if| from being folded away.
> +            bool containsHoistedDecls;
> +            if (!ContainsHoistedDeclaration(cx, discarded, &containsHoistedDecls))

This makes so much more sense to see than isDefn before. I am tempted to go back and request that we try to remove that, as I strongly suspect a RCU cargo-cult failure, in which we made a cargo culted copy of the old code without realizing that we non longer needed that piece, then updated the old code, but not the copy.

@@ +916,5 @@
> +        } 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.

Idgi...

@@ +925,5 @@
> +            // replacement requires folding) or clear it (if |alternative|
> +            // is dead code) as needed.
> +            if (nextNode)
> +                nextNode = (*nextNode == replacement) ? nodePtr : nullptr;
> +            ReplaceNode(nodePtr, replacement);

we can factor this more, if we want, right? ReplaceConditionalNode or something? Probably not worth it, but will avoid some potentially fragile duplication
Attachment #8636896 - Flags: review?(efaustbmo) → review+
Comment on attachment 8639658 [details] [diff] [review]
Fold various simple unary cases by kind

Review of attachment 8639658 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.
Attachment #8639658 - Flags: review?(efaustbmo) → review+
Attachment #8640060 - Flags: review?(efaustbmo)
Attachment #8640062 - Flags: review?(efaustbmo)
Attachment #8640074 - Flags: review?(efaustbmo)
Attachment #8640078 - Flags: review?(efaustbmo)
Attachment #8640081 - Flags: review?(efaustbmo)
Comment on attachment 8639662 [details] [diff] [review]
Fold binary arithmetic operations by kind, not arity

Review of attachment 8639662 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +1170,5 @@
> +
> +    // Repoint the list's tail pointer.
> +    node->pn_tail = listp;
> +
> +    // Now fold all leading numeric terms together into a single number.

please add a note that we can't fold trailing ones because of possible coercion to double.
Attachment #8639662 - Flags: review?(efaustbmo) → review+
Comment on attachment 8636896 [details] [diff] [review]
Fold |if| nodes by kind, with a one-character =/== typo-fix and a targeted test

Review of attachment 8636896 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ -841,5 @@
> -
> -    bool mightHaveHoistedDeclarations = true;
> -
> -    if (false) {
> -      restart:

Nice.
Comment on attachment 8640056 [details] [diff] [review]
Fold various list nodes not given any special treatment

Review of attachment 8640056 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +1634,5 @@
> +      case PNK_ARGSBODY:
> +      case PNK_CALLSITEOBJ:
> +      case PNK_EXPORT_SPEC_LIST:
> +      case PNK_IMPORT_SPEC_LIST:
> +        return FoldList(cx, pn, parser, inGenexpLambda);

The FoldList definition ended up in the addition patch.
Comment on attachment 8640081 [details] [diff] [review]
Fold class nodes by kind

Review of attachment 8640081 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +1632,5 @@
> +{
> +    MOZ_ASSERT(node->isKind(PNK_CLASS));
> +    MOZ_ASSERT(node->isArity(PN_TERNARY));
> +
> +          // The first child of a class is a pair of names referring to it,

Indentation
Comment on attachment 8640056 [details] [diff] [review]
Fold various list nodes not given any special treatment

Review of attachment 8640056 [details] [diff] [review]:
-----------------------------------------------------------------

Shifting a few of the easiest reviews over, for some load-balancing...
Attachment #8640056 - Flags: review?(efaustbmo) → review?(shu)
Comment on attachment 8640074 [details] [diff] [review]
Constant-fold yield/yield*/return by kind

Review of attachment 8640074 [details] [diff] [review]:
-----------------------------------------------------------------

All the structure stuff copies what's in ParseNode.cpp, basically.
Attachment #8640074 - Flags: review?(efaustbmo) → review?(shu)
Comment on attachment 8640081 [details] [diff] [review]
Fold class nodes by kind

Review of attachment 8640081 [details] [diff] [review]:
-----------------------------------------------------------------

.
Attachment #8640081 - Flags: review?(efaustbmo) → review?(shu)
Attachment #8640078 - Flags: review?(efaustbmo) → review?(shu)
Comment on attachment 8640056 [details] [diff] [review]
Fold various list nodes not given any special treatment

Review of attachment 8640056 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah sure why not. I'll assume FoldList does something obvious.
Attachment #8640056 - Flags: review?(shu) → review+
Comment on attachment 8640074 [details] [diff] [review]
Constant-fold yield/yield*/return by kind

Review of attachment 8640074 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +1515,5 @@
> +{
> +    MOZ_ASSERT(node->isKind(PNK_RETURN));
> +    MOZ_ASSERT(node->isArity(PN_BINARY));
> +
> +    if (ParseNode*& expr = node->pn_left) {

Nit: probably clearer to just use &node->pn_left directly below.
Attachment #8640074 - Flags: review?(shu) → review+
Attachment #8640078 - Flags: review?(shu) → review+
Attachment #8640081 - Flags: review?(shu) → review+
Cherrypicked from way down the patch queue for sooner-inspection, given PNK_SUPERPROP and PNK_SUPERELEM are soon going away.
Attachment #8645199 - Flags: review?(efaustbmo)
Comment on attachment 8639663 [details] [diff] [review]
Inline FoldBinaryNumeric into its sole caller and simplify code accordingly

Review of attachment 8639663 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed.

::: js/src/frontend/FoldConstants.cpp
@@ +1061,5 @@
>  }
>  
> +static double
> +ComputeBinary(ParseNodeKind kind, double left, double right)
> +{

Shouldn't there be PNK_POW support here? It looks like there was before. Looks like a rebase mistake? Honestly justified, given the review lag.
Attachment #8639663 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #62)
> Comment on attachment 8639663 [details] [diff] [review]
> Inline FoldBinaryNumeric into its sole caller and simplify code accordingly
> 
> Review of attachment 8639663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed.
> 
> ::: js/src/frontend/FoldConstants.cpp
> @@ +1061,5 @@
> >  }
> >  
> > +static double
> > +ComputeBinary(ParseNodeKind kind, double left, double right)
> > +{
> 
> Shouldn't there be PNK_POW support here? It looks like there was before.
> Looks like a rebase mistake? Honestly justified, given the review lag.

So, I get that it's right associative, so it can't be in this spot, but I would expect to see handling it somewhere?
Comment on attachment 8639665 [details] [diff] [review]
Fold element accesses by kind

Review of attachment 8639665 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +1227,5 @@
> +    if (!Fold(cx, &expr, parser, inGenexpLambda, SyntacticContext::Other))
> +        return false;
> +
> +    ParseNode*& key = node->pn_right;
> +    if (!Fold(cx, &key, parser, inGenexpLambda, SyntacticContext::Other))

This is great. I'm going to wait to rebase the super stuff through this, as it's much easier.

@@ +1277,5 @@
> +    dottedAccess->setInParens(node->isInParens());
> +    ReplaceNode(nodePtr, dottedAccess);
> +
> +    // If we've replaced |expr["prop"]| with |expr.prop|, we can now free the
> +    // |"prop"| and |expr["prop"]| nodes -- but not the |expr| node now a sub-

the second half of this line reads strangely. Perhaps "but not the |expr| node, as it's now..."?
Attachment #8639665 - Flags: review?(efaustbmo) → review+
Comment on attachment 8639670 [details] [diff] [review]
Fold addition by kind

Review of attachment 8639670 [details] [diff] [review]:
-----------------------------------------------------------------

This is really nice.

::: js/src/frontend/FoldConstants.cpp
@@ +1311,5 @@
> +}
> +
> +static bool
> +FoldAdd(ExclusiveContext* cx, ParseNode** nodePtr, Parser<FullParseHandler>& parser,
> +        bool inGenexpLambda)

I like this very much. It's very well laid out, and easy to follow, even for doing a complicated thing.
Attachment #8639670 - Flags: review?(efaustbmo) → review+
Comment on attachment 8640057 [details] [diff] [review]
Fold function calls and tagged templates by kind

Review of attachment 8640057 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +1494,5 @@
> +    //   var prop = "global";
> +    //   var obj = { prop: "obj", f: function() { return this.prop; } };
> +    //   assertEq((true ? obj.f : null)(), "global");
> +    //   assertEq(obj.f(), "obj");
> +    //   assertEq((true ? obj.f : null)``, "global");

This behavior is insane, but not your fault.
Attachment #8640057 - Flags: review?(efaustbmo) → review+
Attachment #8640058 - Flags: review?(efaustbmo) → review+
Attachment #8640060 - Flags: review?(efaustbmo) → review+
Comment on attachment 8640062 [details] [diff] [review]
Constant-fold switch/default by kind

Review of attachment 8640062 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +1701,5 @@
>                 Fold(cx, &pn->pn_right, parser, inGenexpLambda, SyntacticContext::Other);
>  
> +      case PNK_DEFAULT:
> +        MOZ_ASSERT(pn->isArity(PN_BINARY));
> +        MOZ_ASSERT(!pn->pn_left);

What a strange thing to see. Why not use a unary node, then?
Attachment #8640062 - Flags: review?(efaustbmo) → review+
Comment on attachment 8640077 [details] [diff] [review]
Fold for-in, for-of, and for(;;) loops by kind, as well as various miscellaneous things

Review of attachment 8640077 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FoldConstants.cpp
@@ +1770,5 @@
>                 Fold(cx, &pn->pn_right, parser, inGenexpLambda, SyntacticContext::Other);
>  
> +      case PNK_CLASSNAMES:
> +        MOZ_ASSERT(pn->isArity(PN_BINARY));
> +        if (ParseNode*& outerBinding = pn->pn_left) {

prefer to use pn->as<ClassNames>().outerBinding(), or whatever, if we can. Probably not, though, since the types don't line up.

We still have to decide if things are operating over the union, or the substructs. Personally, I favor the latter, but we'll see.
Attachment #8640077 - Flags: review?(efaustbmo) → review+
Attachment #8645199 - Flags: review?(efaustbmo) → review+
And now, finally, the payoffs start to come.  \o/
Attachment #8647211 - Flags: review?(efaustbmo)
Die hack die.
Attachment #8647220 - Flags: review?(efaustbmo)
Attachment #8647221 - Flags: review?(efaustbmo)
Comment on attachment 8647211 [details] [diff] [review]
Remove dead fold-by-arity code

Review of attachment 8647211 [details] [diff] [review]:
-----------------------------------------------------------------

+1
Attachment #8647211 - Flags: review?(efaustbmo) → review+
Comment on attachment 8647213 [details] [diff] [review]
Remove SyntacticContext::Delete, now addressed by modifying how delete nodes are folded

Review of attachment 8647213 [details] [diff] [review]:
-----------------------------------------------------------------

Yuuup
Attachment #8647213 - Flags: review?(efaustbmo) → review+
Comment on attachment 8647214 [details] [diff] [review]
Replace callee-based condition-constant folding with caller-specified condition-constant folding

Review of attachment 8647214 [details] [diff] [review]:
-----------------------------------------------------------------

I guess the removal of SyntacticContext::Condition happens in a different patch?
Attachment #8647214 - Flags: review?(efaustbmo) → review+
Comment on attachment 8647215 [details] [diff] [review]
Remove special |void| handling by making Boolish recognize |void| expressions as falsy, when they're obviously so

Review of attachment 8647215 [details] [diff] [review]:
-----------------------------------------------------------------

See comment below for a thought. I don't know how good a thought it is, but it's sure a thought.

::: js/src/frontend/FoldConstants.cpp
@@ +486,5 @@
>        case PNK_NUMBER:
>          return (pn->pn_dval != 0 && !IsNaN(pn->pn_dval)) ? Truthy : Falsy;
>  
>        case PNK_STRING:
> +      case PNK_TEMPLATE_STRING:

This can live here, but it's unrelated to this patch, right?

@@ +518,5 @@
> +            pn->isKind(PNK_NULL) ||
> +            pn->isKind(PNK_FUNCTION) ||
> +            pn->isKind(PNK_GENEXP))
> +        {
> +            return Falsy;

I don't really care that much, but I feel like this could also be

case PNK_VOID:
  if (Boolish(pn->pn_kid) == Unknown)
    return Unknown;
  return Falsy;

(You could still iterate down the void chain to avoid bad recursion)

Probably this is too fragile? But we have a pretty good guarantee about the side effect free nature, if we can tell exactly what they evaluate to, I would think.
Attachment #8647215 - Flags: review?(efaustbmo) → review+
Comment on attachment 8647216 [details] [diff] [review]
Remove SyntacticContext::Condition, now handled context-sensitively by callers specifically requesting condition-targeted folding

Review of attachment 8647216 [details] [diff] [review]:
-----------------------------------------------------------------

WFM
Attachment #8647216 - Flags: review?(efaustbmo) → review+
Comment on attachment 8647220 [details] [diff] [review]
Remove SyntacticContext completely

Review of attachment 8647220 [details] [diff] [review]:
-----------------------------------------------------------------

Well done.
Attachment #8647220 - Flags: review?(efaustbmo) → review+
Comment on attachment 8647221 [details] [diff] [review]
Common up some is-effectless testing

Review of attachment 8647221 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8647221 - Flags: review?(efaustbmo) → review+
Keywords: leave-open
Duplicate of this bug: 626518
You need to log in before you can comment on or make changes to this bug.