The default bug view has changed. See this FAQ.

IonMonkey: Compile JSOP_GETPROP as a call to the VM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Necessary for benchmarks.
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)
OS: Linux → All
Hardware: x86_64 → All
Summary: IonMonkey: Compile JSOP_GETPROP & JSOP_SETPROP → IonMonkey: Compile JSOP_GETPROP as a call to the VM
Blocks: 707854
Blocks: 707856
(Assignee)

Updated

5 years ago
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: 708441
(Assignee)

Comment 2

5 years ago
Created attachment 580559 [details] [diff] [review]
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.
Attachment #580559 - Flags: review?(dvander)
(Assignee)

Comment 3

5 years ago
Created attachment 581392 [details] [diff] [review]
Implement JSOP_GETPROP as a VMFunction.

Rebased and fix C function calls to accept different number of arguments.
Attachment #580559 - Attachment is obsolete: true
Attachment #580559 - Flags: review?(dvander)
Attachment #581392 - Flags: review?(dvander)
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.
Attachment #581392 - Flags: review?(dvander) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 581544 [details] [diff] [review]
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.
Attachment #581392 - Attachment is obsolete: true
Attachment #581544 - Flags: review?(dvander)
Attachment #581544 - Flags: review?(dvander) → review+
This seems to have been in the tree since thursday, but there has been no update this the bug.
http://hg.mozilla.org/projects/ionmonkey/rev/6016a42c99d9
(Assignee)

Comment 8

5 years ago
Thanks for updating, I am on my way to get it working on ARM (Bug 710130).
Closing this one.
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.