Closed Bug 735391 Opened 12 years ago Closed 12 years ago

IonMonkey: Inline Math.sqrt

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: djvj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Should be straightforward, we can re-use the native inlining infrastructure for Math.abs and friends. Math.sqrt can be implemented using sqrtsd on x86/x64 and vsqrt on ARM.
Assignee: general → evilpies
Assignee: evilpies → general
Assignee: general → dvander
Status: NEW → ASSIGNED
Assignee: dvander → kvijayan
Attached patch Patch v1Splinter Review
About a 27% speed improvement (avg across 10 runs) on the following
microbench on x86-64:

function foo(j) {
    return Math.sqrt(j) + Math.sqrt(j + 2);
}
var arr = [];
for(var i = 0; i < 50000000; i++) {
    arr.push(foo(i * 0.95));
}
for(var i = 1; i < arr.length; i *= 2) {
    print(arr[i]);
}
Attachment #607339 - Flags: review?(dvander)
Comment on attachment 607339 [details] [diff] [review]
Patch v1

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

::: js/src/ion/MCallOptimize.cpp
@@ +148,5 @@
> +                if (arg1Type == MIRType_Int32) {
> +                    MToDouble *conv = MToDouble::New(input);
> +                    current->add(conv);
> +                    current->push(conv);
> +                    input = conv;

Here, I would not do this conversion. Instead just pass the input wholesale and the type conversion process will do the rest.

::: js/src/ion/MIR.h
@@ +1912,5 @@
> +{
> +    MSqrt(MDefinition *num)
> +      : MUnaryInstruction(num)
> +    {
> +        JS_ASSERT(num->type() == MIRType_Double);

Here, I would set specialization_ = MIRType_Double instead of this assert. Then ArithPolicy will guarantee conversion for you.
Attachment #607339 - Flags: review?(dvander) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I don't see a fix for this or a link to the mercurial commit. Has this optimization been applied?
You need to log in before you can comment on or make changes to this bug.