701958 - IonMonkey: Compile JSOP_GETPROP as a call to the VM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Necessary for benchmarks.

Comment 1

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

These are fairly different opcodes, and both are complex, so I'm going to morph this. Brian wants to start on ICs, so the first step is making sure we can call out to C++ functions like this. The signature should look something like:

bool GetProperty(JSContext *cx, JSObject *obj, JSAtom *atom, Value *vp)

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Implement JSOP_GETPROP as a VMFunction.

Use callVM for GETPROP implementation.
 - not Idempotent & avoid GVN on unboxing of the result.
 - Call the generic function to getproperties
 - Extract the GetProperty function from the interpreter.

This patch does not pass the whole test-suite yet.  I have to investigate the 32 failures of "--ion-tbpl --ion --no-slow js ion" to determine if this is directly caused by this patch.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Implement JSOP_GETPROP as a VMFunction.

Rebased and fix C function calls to accept different number of arguments.

Comment 4

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

Comment on attachment 581392 [details] [diff] [review]
Implement JSOP_GETPROP as a VMFunction.

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

r=me, but with some changes:

::: js/src/ion/IonBuilder.cpp
@@ +2878,5 @@
> +        return false;
> +
> +    types::TypeSet *barrier;
> +    types::TypeSet *types = oracle->returnTypeSet(script, pc, &barrier);
> +    return pushTypeBarrier(ins, types, barrier);

returnTypeSet() is for the result of JS->JS calls, so here you want to use TypeOracle::propertyRead and TypeOracle::propertyReadBarrier.

::: js/src/ion/LIR-Common.h
@@ +1019,5 @@
>          return getOperand(1);
>      }
>  };
>  
> +class LLoadPropertyV : public LVMCallInstructionHelper<LDefinition::BOX, BOX_PIECES, 1, 0>

LLoadPropertyV will be the fast GetProp, so this deserves a name like LLoadPropertyGeneric or something. You can omit the V because there is no typed version.

::: js/src/ion/LIR.h
@@ +820,2 @@
>              JS_ASSERT(Defs == BOX_PIECES);
>              return Registers::JSCallMask;

You'll probably have to take the #ifdef PUNBOX64 off LDefinition::BOX. Alternately, we could remove this template parameter and normalize all of the return registers to CallMask/JSCallMask, thus making it possible to switch on Defs.

::: js/src/ion/MIR.h
@@ +2038,5 @@
>      }
>  };
>  
> +// Load from vp[slot] (slots that are not inline in an object).
> +class MLoadProperty

Since this is the slowest of the slow versions of the op, let's give it a scarier name, like "MLoadPropertyGeneric" (s/Load/Get/, s/Generic/Slow would be okay too).

@@ +2042,5 @@
> +class MLoadProperty
> +  : public MUnaryInstruction,
> +    public ObjectPolicy
> +{
> +    HeapPtr<JSAtom> atom_;

No need for HeapPtr<> here, since we don't trace gcthings in MIR.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +764,5 @@
>  
>  bool
>  CodeGeneratorX86Shared::visitNewArray(LNewArray *ins)
>  {
> +    static const VMFunction NewInitArrayInfo = FunctionInfo(NewInitArray);

Talked in IRL about this.

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +279,5 @@
>          movq(Operand(address), dest);
>      }
> +
> +    using MacroAssemblerX86Shared::Push;
> +    void Push(const ImmGCPtr ptr, Register tmp = ScratchReg) {

Remove the second parameter here (it's unused, and conflicts with the x86/ARM signatures).

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +314,5 @@
> +
> +    using MacroAssemblerX86Shared::Push;
> +    void Push(const ImmGCPtr ptr) {
> +        Push(Imm32(ptr.value));
> +        writeDataRelocation(masm.currentOffset());

Instead, can you add: void push(ImmGCPtr ptr) to Assembler-x86, and perform the relocation there? Let's hide as much of the relocation work as we can in low-level assembler.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Implement JSOP_GETPROP as a VMFunction.

Fix type inference: use propertyRead instead of returnTypeSet
Remove implicit template specialization (typedef close to the push of arguments)
Remove useless HeapPtr: No GC during compilation.
Move relocation logic to the Assembler files.
Rename LoadPropertyV to LoadPropertyGeneric.
Rebase and update VMFunction & FunctionInfo.

Comment 6

[Brian Hackett [Laid off!]]

This seems to have been in the tree since thursday, but there has been no update this the bug.

Comment 7

[Tom S [:evilpie]]

http://hg.mozilla.org/projects/ionmonkey/rev/6016a42c99d9

Comment 8

[Nicolas B. Pierron [:nbp]]

Thanks for updating, I am on my way to get it working on ARM (Bug 710130).
Closing this one.