IonMonkey: IC for JSOP_GETELEM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
For string-fasta, we need an IC to handle obj[string].
(Assignee)

Comment 1

5 years ago
Created attachment 605359 [details] [diff] [review]
Patch

This guards on the index value and then generates a normal getprop stub. The patch is pretty big, but most of it is just moving code around.
Attachment #605359 - Flags: review?(dvander)
Comment on attachment 605359 [details] [diff] [review]
Patch

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

::: js/src/ion/IonBuilder.cpp
@@ +3396,5 @@
> +    if (oracle->elementReadIsCacheable(script, pc, &mustMonitorResult)) {
> +        ins = MGetElementCache::New(lhs, rhs, mustMonitorResult);
> +    } else {
> +        ins = MCallGetElement::New(lhs, rhs);
> +        mustMonitorResult = true;

Maybe slightly nicer if mustMonitorResult was set by the Oracle in all case (if that makes the API look weird, it could be void and have two outparams: cacheable, mustMonitor).

::: js/src/ion/TypePolicy.cpp
@@ +331,5 @@
> +    {
> +        return true;
> +    }
> +
> +    // Once we have C++ calls, we can change this to insert a PrimitiveToObject

While you're here, could you remove this comment? It's not necessarily correct, so it's misleading.

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +328,5 @@
>      }
>      Condition testGCThing(Condition cond, const ValueOperand &value) {
>          return testGCThing(cond, value.typeReg());
>      }
> +    void branchTestValue(Condition cond, const ValueOperand &value, const Value &v, Label *label);

Needs ARM parity.
Attachment #605359 - Flags: review?(dvander) → review+
(Assignee)

Updated

5 years ago
Blocks: 735699
(Assignee)

Comment 3

5 years ago
Created attachment 606301 [details] [diff] [review]
Patch v2

Marty, can you review the MacroAssembler-arm changes?

Can you double check whether the branchTestValue function does the right thing both if cond is Equal or NotEqual? I think it does, but I'm not 100% sure about the ARM condition codes.
Attachment #605359 - Attachment is obsolete: true
Attachment #606301 - Flags: review?(mrosenberg)
Comment on attachment 606301 [details] [diff] [review]
Patch v2

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

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1639,5 @@
> +    if (v.isMarkable())
> +        ma_cmp(value.payloadReg(), ImmGCPtr(reinterpret_cast<gc::Cell *>(v.toGCThing())));
> +    else
> +        ma_cmp(value.payloadReg(), Imm32(jv.s.payload.i32));
> +    ma_cmp(value.typeReg(), Imm32(jv.s.tag), Equal);

The compare on equal looks correct, but it took me a while to work out, perhaps a comment stating the logic behind it.
cond == NotEqual => branch when a.tag != b.tag || a.value != b.value => short circuit true (NotEqual) => compare values when tags equal
cand == Equal => branch when a.tag == b.tag && a.value == b.value => short circuit false (NotEqual) => compare values when tags equal.
Attachment #606301 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 5

5 years ago
Pushed with comments addressed:

http://hg.mozilla.org/projects/ionmonkey/rev/f5ec9bd2b017
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.