IonMonkey: LoadElementT should support hole checks

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ion:p2:fx18])

Attachments

(2 attachments)

Assignee

Description

7 years ago
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.
Assignee

Updated

7 years ago
Blocks: 768572
Assignee

Comment 1

7 years ago
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]
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.
Attachment #637895 - Flags: review?(sstangl) → review+

Comment 3

7 years ago
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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5082df10222
https://hg.mozilla.org/mozilla-central/rev/e5082df10222
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.