Closed
Bug 734383
Opened 12 years ago
Closed 12 years ago
IonMonkey: IC for JSOP_GETELEM
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
44.46 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
For string-fasta, we need an IC to handle obj[string].
Assignee | ||
Comment 1•12 years ago
|
||
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 | ||
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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•12 years ago
|
||
Pushed with comments addressed: http://hg.mozilla.org/projects/ionmonkey/rev/f5ec9bd2b017
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•