Last Comment Bug 701958 - IonMonkey: Compile JSOP_GETPROP as a call to the VM
: IonMonkey: Compile JSOP_GETPROP as a call to the VM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 684381 707854 707856 708441
  Show dependency treegraph
 
Reported: 2011-11-11 17:54 PST by Nicolas B. Pierron [:nbp]
Modified: 2011-12-19 10:16 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement JSOP_GETPROP as a VMFunction. (18.11 KB, patch)
2011-12-09 14:45 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Implement JSOP_GETPROP as a VMFunction. (23.02 KB, patch)
2011-12-13 13:24 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review
Implement JSOP_GETPROP as a VMFunction. (31.14 KB, patch)
2011-12-13 22:30 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-11-11 17:54:21 PST
Necessary for benchmarks.
Comment 1 David Anderson [:dvander] 2011-12-05 17:56:47 PST
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] 2011-12-09 14:45:28 PST
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.
Comment 3 Nicolas B. Pierron [:nbp] 2011-12-13 13:24:17 PST
Created attachment 581392 [details] [diff] [review]
Implement JSOP_GETPROP as a VMFunction.

Rebased and fix C function calls to accept different number of arguments.
Comment 4 David Anderson [:dvander] 2011-12-13 14:02:52 PST
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] 2011-12-13 22:30:07 PST
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.
Comment 6 Brian Hackett (:bhackett) 2011-12-19 08:50:34 PST
This seems to have been in the tree since thursday, but there has been no update this the bug.
Comment 7 Tom Schuster [:evilpie] 2011-12-19 09:01:11 PST
http://hg.mozilla.org/projects/ionmonkey/rev/6016a42c99d9
Comment 8 Nicolas B. Pierron [:nbp] 2011-12-19 10:16:08 PST
Thanks for updating, I am on my way to get it working on ARM (Bug 710130).
Closing this one.

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