570805 - JM: Implement JSOP_CALLPROP

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: Implement JSOP_CALLPROP.

Implements JSOP_CALLPROP.

The code diverges from what is used by the interpreter: the section of code shared with JSOP_GETPROP is not duplicated; stubs::GetProp() is called explicitly by stubs::CallProp(). The previous iteration of JaegerMonkey handles CallProp() in a special way for PIC usage, but without PICs, the implementation this patch provides is as clean as possible.

The code passes all tests except for sunspider/check-crypto-md5.js and sunspider/check-crypto-sha1.js, which trip the assertion:

Assertion failure: !avail.empty(), at ../methodjit/FrameState.cpp:288

This assertion is unrelated to the correctness of the patch, but blocks this patch if we don't want to burn the tree.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 449927 [details] [diff] [review]
Implement JSOP_CALLPROP.


>+          BEGIN_CASE(JSOP_CALLPROP)
>+            prepareStubCall();
>+            stubCall(stubs::CallProp, Uses(1), Defs(2));
>+            frame.pushSynced();
>+          END_CASE(JSOP_CALLPROP)

This should be frame.pop() ; frame.pushSynced() ; frame.pushSynced()
Reason is, the top of the stack might be a propagated copy or constant, and then the JIT is using the wrong value. Eventually we want some sort of frame.swap() - after or during PICs.

>+    GetProp(f);

This is fallible, so some sort of check is needed. Splitting GetProp into a helper might be best.

>+    if (JS_UNLIKELY(rval.isUndefined())) {
>+        /* inline jsinterp.cpp:LOAD_ATOM */
>+        JSAtom *atom = f.fp->script->atomMap.vector[GET_INDEX(regs.pc)];

Let's pass the atom as an inparam, as it's a compile-time constant. See jsop_propinc for an example of how to do this. Long term goal is to remove regs.pc.

Comment 2

[Sean Stangl [:sstangl]]

Attachment: Original patch with above changes.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 449961 [details] [diff] [review]
Original patch with above changes.


>+    if (!InlineGetProp(f))
>+        THROW();

Double throw here. Better to leave out THROW() in InlineGetProp, and propagate the return value up. THROW()ing only inside the actual stub calls makes the control flow more explicit.

Thanks!

Comment 4

[Sean Stangl [:sstangl]]

Pushed.