727857 - IonMonkey: LoadElementT should support hole checks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

At the moment when a GETELEM needs a hole check, we don't support typed loads and fall back to LoadElementV. This requires an extra type register on x86, increasing register pressure in v8-crypto's am3 function.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Adds support for hole checks to LoadElementT. The x86 implementation is straight-forward, a single comparison with a memory operand. x64 are ARM require a bit more work.

Comment 2

[Sean Stangl [:sstangl]]

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

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

r+ with x64 changes.

::: js/src/ion/x64/CodeGenerator-x64.cpp
@@ +286,1 @@
>      Operand source = createArrayElementOperand(ToRegister(load->elements()), load->index());

I think this entire method can be more simply expressed as:

> if (load->mir()->needsHoleCheck()) {
>     masm.splitTag(source, ScratchReg);
>     Assembler::Condition cond = masm.testMagic(Assembler::Equal, ScrachReg);
>     if (!bailoutIf(cond, load->snapshot()))
>         return false;
> }
> loadUnboxedValue(source, load->mir()->type(), load->output());

The extra code here seems to just be duplicating the logic in loadUnboxedValue() to avoid a few instructions.
The simpler version looks fine to me.

@@ +315,5 @@
> +        masm.bind(&done);
> +    } else {
> +        // Load Value.
> +        ValueOperand val = ValueOperand(output.gpr());
> +        masm.loadValue(source, val);

It's unsafe to clobber an outreg before a bailout.

Comment 3

[Paul Wright]

Since this blocks a P1 bug, is this being worked on?

Comment 4

[Brian Hackett [Laid off!]]

Attachment: x86 patch

Update patch and remove non-x86 paths for LoadElementT.  There doesn't seem to be a lot of benefit to doing this on ARM or x64 --- ARM can load both type and payload in one instruction, and x64 needs to load into the scratch reg anyways to test an in memory type tag.

Comment 5

[Marty Rosenberg [:mjrosenb]]

>   There doesn't seem
> to be a lot of benefit to doing this on ARM or x64 --- ARM can load both
> type and payload in one instruction,

This is mostly true.  There is an instruction that will load 64 bits from memory into two consecutive even-aligned registers, but this requires the register allocator to have been nice to us, which I suspect does not happen that frequently.

Comment 6

[Brian Hackett [Laid off!]]

Pushed, with a tweak --- checking how to handle typed hole reads during lowering doesn't work on x64 as unbox instructions aren't properly inserted, so this checks instead during IonBuilder.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5082df10222

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5082df10222