IonMonkey: LoadElementT should support hole checks

RESOLVED FIXED in mozilla20



8 years ago
7 years ago


(Reporter: jandem, Assigned: jandem)


(Blocks 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [ion:p2:fx18])


(2 attachments)

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.
Blocks: 768572
Posted patch PatchSplinter Review
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.
Attachment #637895 - Flags: review?(sstangl)
Comment on attachment 637895 [details] [diff] [review]

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.
Attachment #637895 - Flags: review?(sstangl) → review+
Since this blocks a P1 bug, is this being worked on?
Posted patch x86 patchSplinter Review
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.
Attachment #687241 - Flags: review?(sstangl)
Attachment #687241 - Flags: review?(sstangl) → review+
>   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.
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.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.