Last Comment Bug 734383 - IonMonkey: IC for JSOP_GETELEM
: IonMonkey: IC for JSOP_GETELEM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on:
Blocks: IonSpeed 735699
  Show dependency treegraph
 
Reported: 2012-03-09 07:09 PST by Jan de Mooij [:jandem]
Modified: 2012-03-16 12:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (42.29 KB, patch)
2012-03-13 05:28 PDT, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review
Patch v2 (44.46 KB, patch)
2012-03-15 11:18 PDT, Jan de Mooij [:jandem]
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-03-09 07:09:27 PST
For string-fasta, we need an IC to handle obj[string].
Comment 1 Jan de Mooij [:jandem] 2012-03-13 05:28:53 PDT
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.
Comment 2 David Anderson [:dvander] 2012-03-13 19:19:25 PDT
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.
Comment 3 Jan de Mooij [:jandem] 2012-03-15 11:18:42 PDT
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.
Comment 4 Marty Rosenberg [:mjrosenb] 2012-03-16 03:10:12 PDT
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.
Comment 5 Jan de Mooij [:jandem] 2012-03-16 12:19:52 PDT
Pushed with comments addressed:

http://hg.mozilla.org/projects/ionmonkey/rev/f5ec9bd2b017

Note You need to log in before you can comment on or make changes to this bug.