1323096 - CacheIR: optimize more value[double] cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Jan de Mooij [:jandem]]

Bug 1322091 part 6 changed GuardIsInt32 to also check for doubles and convert these in-place to int32 if possible.

We use this for the typed array case, but it's now trivial to support value[int32-stored-as-double] for all of these stubs, we just need to check for this case when we attach the stub.

It would be nice to check this in tryAttachStub and then pass |int32_t index| to tryAttachDenseElement. Then we can also remove the indexVal_.isInt32() asserts.

Marking as P1: easy fix, improves IC coverage so it can be a big perf win in some cases, and we should be able to fix this before the end of the year.

Comment 1

[Jan de Mooij [:jandem]]

Ion's typed/unboxed array stub also supports object[str] where str is an integer-string. We should probably support this in GuardIsInt32 as well, so we don't regress our Ion IC coverage. And (same story every time) then this will work for all stubs/JITs.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> We should probably support this in GuardIsInt32 as well, so
> we don't regress our Ion IC coverage.

After submitting this, I'm thinking we shouldn't do this in GuardIsInt32 but add a new op for that (or rename GuardIsInt32 to GuardIsInt32Key or so). The reason is that GuardIsInt32 may later be used for something else, like |int32 + int32|, and then the difference will be observable.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

With this patch, we are able to use all object[int32] stubs for object[double] (when the double is an integer) and object[string] (when the string is an int32 index).

The string case comes up with for-in in a few benchmarks. The micro-benchmark below is much faster now, especially with --no-ion (5 seconds => 1.5 seconds).

I didn't change the StringChar and MagicArguments stubs. Lazy arguments are only used if the index is int32, and StringChar is a little annoying because we have to restructure the code to first check everything and then emit the IR for the guards. It's not that difficult to fix, but not worth it now IMO.

function f(a) {
    var a = new Int32Array(100);
    var res;
    for (var i=0; i<100000; i++) {
        for (var j in a)
            res = a[j] + a[j] + a[j];
    }
    return res;
}
var t = new Date;
f();
print(new Date - t);

Comment 4

[Tom S [:evilpie]]

Comment on attachment 8822069 [details] [diff] [review]
Patch

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

Bug 654190 would be cool to have for the string path. But still a nice improvement. It's so nice to have a central way of improving this.

::: js/src/jit/CacheIR.cpp
@@ +1298,5 @@
> +}
> +
> +bool
> +IRGenerator::maybeGuardInt32Index(Value index, ValOperandId indexId,
> +                                  int32_t* int32Index, Int32OperandId* int32IndexId)

It doesn't really fit the name, but using uint32_t would be more useful for the try functions.

::: js/src/jit/CacheIRCompiler.h
@@ +27,5 @@
>      _(GuardNoUnboxedExpando)              \
>      _(GuardAndLoadUnboxedExpando)         \
>      _(GuardNoDetachedTypedObjects)        \
>      _(GuardNoDenseElements)               \
> +    _(GetInt32IndexFromString)            \

GuardAndGet[Int32]IndexFromString, not sure if we want to drop the Int32, it's technically Int31 right now ;)

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #4)
> It doesn't really fit the name, but using uint32_t would be more useful for
> the try functions.

Done.

> > +    _(GetInt32IndexFromString)            \
> 
> GuardAndGet[Int32]IndexFromString, not sure if we want to drop the Int32,
> it's technically Int31 right now ;)

Renamed it to GuardAndGetIndexFromString.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05a6f5bcb7a0
Optimize object[double] and object[string] like object[int32]. r=evilpie

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/05a6f5bcb7a0