Last Comment Bug 781368 - IonMonkey: Fold TypeOf strings in MConstant.
: IonMonkey: Fold TypeOf strings in MConstant.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-08 15:45 PDT by Sean Stangl [:sstangl]
Modified: 2012-08-08 16:32 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.31 KB, patch)
2012-08-08 15:45 PDT, Sean Stangl [:sstangl]
no flags Details | Diff | Splinter Review
patch v2 (2.78 KB, patch)
2012-08-08 16:02 PDT, Sean Stangl [:sstangl]
efaustbmo: review+
Details | Diff | Splinter Review

Description Sean Stangl [:sstangl] 2012-08-08 15:45:06 PDT
Created attachment 650345 [details] [diff] [review]
patch

JSOP_TYPEOF can produce an MConstant string, but we still lower to LCompareS, which takes a VM call. Instead, we should just fold away the comparison where possible.

Miniscule performance improvement on kraken-crypto-pbkdf2.
Comment 1 Eric Faust [:efaust] 2012-08-08 15:55:41 PDT
Comment on attachment 650345 [details] [diff] [review]
patch

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

::: js/src/ion/MIR.cpp
@@ +1354,5 @@
> +        if (left == right)
> +            equal = true;
> +        else if (!EqualStrings(GetIonContext()->cx, lhs.toString(), rhs.toString(), &equal))
> +            return false;
> +        *result = (equal ^ (jsop_ != JSOP_EQ && jsop_ != JSOP_STRICTEQ));

So if jsop_ == JSOP_LT, and equal came back false, result must be true? Are we guaranteed of that ordering?
Comment 2 Sean Stangl [:sstangl] 2012-08-08 16:02:55 PDT
Created attachment 650350 [details] [diff] [review]
patch v2

Whoops. Calling EqualStrings() is obviously only a useful optimization if we're going to be checking whether strings are equal.
Comment 3 Eric Faust [:efaust] 2012-08-08 16:10:56 PDT
Comment on attachment 650350 [details] [diff] [review]
patch v2

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

r=me.

::: js/src/ion/Lowering.cpp
@@ +351,5 @@
> +        return add(new LGoto(num ? ifTrue : ifFalse));
> +    }
> +
> +    // Constant Boolean operand.
> +    if (opd->type() == MIRType_Boolean && opd->isConstant()) {

In theory it makes more sense to test ops->isConsant() first, since that's less likely to be true, but this is more in line with the structure of the existing code.

::: js/src/ion/MIR.cpp
@@ +1360,5 @@
> +        if (left == right)
> +            equal = true;
> +        else if (!EqualStrings(GetIonContext()->cx, lhs.toString(), rhs.toString(), &equal))
> +            return false;
> +        *result = (equal ^ (jsop_ != JSOP_EQ && jsop_ != JSOP_STRICTEQ));

We could still make JSOP_GE and JSOP_LE work here in the equal case, at least. At any rate, this could use a comment.
Comment 4 Sean Stangl [:sstangl] 2012-08-08 16:32:55 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/0400c12c5295

Note You need to log in before you can comment on or make changes to this bug.