1944843 - GuardToInt32Index isn't optimized well in Ion for boxed Values

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task)

Description

[Jan de Mooij [:jandem]]

For an array access such as array[index] we use the CacheIR op GuardToInt32Index for the index. This handles both int32 and double values but we use this also when we only see int32 indexes.

If the index is a boxed value, this op is transpiled to MToNumberInt32 which is lowered to LValueToNumberInt32.

The machine code we end up generating for the g function below contains this:

[LoadFixedSlotV]
movq       0x18(%rax), %rdx
...
[ValueToNumberInt32]
movq       %rdx, %r11
shrq       $47, %r11
cmpl       $0x1fff1, %r11d
je         .Lfrom121
cmpl       $0x1fff0, %r11d
jbe        .Lfrom134
jmp        .Lfrom139
vmovq      %rdx, %xmm0
vcvttsd2si %xmm0, %eax
xorpd      %xmm15, %xmm15
cvtsi2sd   %eax, %xmm15
vucomisd   %xmm15, %xmm0
jp         .Lfrom169
jne        .Lfrom175
jmp        .Lfrom180
movl       %edx, %eax

This has two problems:

1. It's a lot of machine code for a plain int32 index (we could move the code for non-int32 values to an OOL path).
2. If this was an int32 MUnbox Ion would have optimized this to LoadFixedSlotAndUnbox.

We should try to change the CacheIR generator to use an int32 guard instead of GuardToInt32Index when it sees an int32 index. It would also be interesting to collect some data for Sp3/JetStream on how often we see a double value for this instruction.

function g(arr, o) {
  return arr[o.index];
}
function f() {
  with (this);
  var arr = [1, 2, 3];
  var o = {index: 1};
  for (var i = 0; i < 100_000; i++) {
    res = g(arr, o);
  }
  return res;
}
f();

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1944843 part 1 - Preserve index register in CacheIRCompiler::emitCallObjectHasSparseElementResult. r?iain!

The next patch affects CacheIR register allocation and this caused a jit-test failure
that's fixed by this.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1944843 part 2 - Use GuardToInt32 instead of GuardToInt32Index for int32 values. r?iain!

This results in better machine code in Ion when the index is a boxed Value because
we now only handle double indexes after we've seen one. This is more consistent with
what we do in other places.

Improves JetStream's Crypto test from Octane by 10-20% on some platforms.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f9fd144d4b0
part 1 - Preserve index register in CacheIRCompiler::emitCallObjectHasSparseElementResult. r=iain
https://hg.mozilla.org/integration/autoland/rev/31a6e5abd49b
part 2 - Use GuardToInt32 instead of GuardToInt32Index for int32 values. r=iain

Comment 4

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7f9fd144d4b0
https://hg.mozilla.org/mozilla-central/rev/31a6e5abd49b

Comment 5

[Mayank Bansal]

Jetstream2:
21.3% on crypto-geometric
5% on pdf-js
3.3% on stanford-crypto-aes

Octane:
21% on crypto

2.5% on Kraken-stanford-crypto-aes