Last Comment Bug 691340 - IonMonkey: Implement JSOP_INITELEM
: IonMonkey: Implement JSOP_INITELEM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on: 680315 692838
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-10-03 08:10 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-02-21 22:11 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[0001] Add number of registers used to ship boxed values. (1.16 KB, patch)
2011-10-03 10:45 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review
[0002] Make ToValue function visible inside CodeGeneratorShared class. (769 bytes, patch)
2011-10-03 10:46 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review
Make JSObject private fields visible inside the CodeGeneratorX86Shared. (830 bytes, patch)
2011-10-03 10:47 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review
[0004] Add type-set Type policy to optimize based on type inference. (4.61 KB, patch)
2011-10-03 10:49 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
[0005] Handle non-optimized JSOP_INITELEM bytecode instruction. (10.59 KB, patch)
2011-10-03 10:50 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
[0006] Add optimized version of JSOP_INITELEM. (7.76 KB, patch)
2011-10-03 10:51 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
[0004] Add prototype-policy: use a complex prototype for the type policy of MIR Instructions. (9.25 KB, patch)
2011-11-11 17:36 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
[0005] Add optimized version of JSOP_INITELEM. (16.59 KB, patch)
2011-11-11 17:37 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Implement JSOP_INITELEM. (16.19 KB, patch)
2011-12-30 17:43 PST, Nicolas B. Pierron [:nbp]
jdemooij: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-10-03 08:10:49 PDT
+++ This bug was initially created as a clone of Bug #685838 +++

Implement the opcode JSOP_INITELEM in ionmonkey.
Comment 1 Nicolas B. Pierron [:nbp] 2011-10-03 10:45:17 PDT
Created attachment 564242 [details] [diff] [review]
[0001] Add number of registers used to ship boxed values.
Comment 2 Nicolas B. Pierron [:nbp] 2011-10-03 10:46:13 PDT
Created attachment 564244 [details] [diff] [review]
[0002] Make ToValue function visible inside CodeGeneratorShared class.
Comment 3 Nicolas B. Pierron [:nbp] 2011-10-03 10:47:02 PDT
Created attachment 564246 [details] [diff] [review]
Make JSObject private fields visible inside the CodeGeneratorX86Shared.
Comment 4 Nicolas B. Pierron [:nbp] 2011-10-03 10:49:43 PDT
Created attachment 564251 [details] [diff] [review]
[0004] Add type-set Type policy to optimize based on type inference.

This patch may likely be put inside another bug on which many instruction may depends on.  In the mean time I add it here because it is used by patch 6.
Comment 5 Nicolas B. Pierron [:nbp] 2011-10-03 10:50:27 PDT
Created attachment 564252 [details] [diff] [review]
[0005] Handle non-optimized JSOP_INITELEM bytecode instruction.
Comment 6 Nicolas B. Pierron [:nbp] 2011-10-03 10:51:34 PDT
Created attachment 564254 [details] [diff] [review]
[0006] Add optimized version of JSOP_INITELEM.
Comment 7 David Anderson [:dvander] 2011-10-05 15:39:08 PDT
Comment on attachment 564246 [details] [diff] [review]
Make JSObject private fields visible inside the CodeGeneratorX86Shared.

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

::: js/src/jsobj.h
@@ +1133,4 @@
>    private:
>      friend struct JSFunction;
>      friend class js::mjit::Compiler;
> +    friend class js::ion::CodeGeneratorX86Shared;

Will this break on ARM? Would it work to have CodeGeneratorShared as a friend instead?

Another option is to expose static offsetOf accessors.
Comment 8 David Anderson [:dvander] 2011-10-05 15:43:07 PDT
Comment on attachment 564242 [details] [diff] [review]
[0001] Add number of registers used to ship boxed values.

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

::: js/src/ion/x86/Assembler-x86.h
@@ +69,4 @@
>  static const Register JSCReturnReg_Type = eax;
>  static const Register JSCReturnReg_Data = edx;
>  static const FloatRegister ScratchFloatReg = { JSC::X86Registers::xmm7 };
> +static const uint32 NumJSReg = 2;

This name is kind of mysterious - how about NumRegsPerValue? (or NumValueRegs)?
Comment 9 Nicolas B. Pierron [:nbp] 2011-10-07 07:22:29 PDT
(In reply to David Anderson [:dvander] from comment #7)
> Comment on attachment 564246 [details] [diff] [review] [diff] [details] [review]
> Make JSObject private fields visible inside the CodeGeneratorX86Shared.
> 
> Review of attachment 564246 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsobj.h
> @@ +1133,4 @@
> >    private:
> >      friend struct JSFunction;
> >      friend class js::mjit::Compiler;
> > +    friend class js::ion::CodeGeneratorX86Shared;
> 
> Will this break on ARM? Would it work to have CodeGeneratorShared as a
> friend instead?

At least with gcc this does not seems to cause any problem when we have both forward declarations of CodeGeneratorX86 and CodeGeneratorX64 and when both are declared as friend.

I guess this should not cause any problem because the "friend" keyword is used by the compiler to inform that a named class has the right to access the private member of the current class.

Unfortunately friendship is not inherited in C++.  Thus adding CodeGeneratorShared as friend would not factor this.  I had to add both CodeGeneratorX86 and CodeGeneratorX64 when I added the implementation for x86 and x64.

I was looking to factor this with macros, but the current implementation would be larger with macros than it is in plain text.

> Another option is to expose static offsetOf accessors.

I didn't check how many times we would need to implement such offsetOf functions to make JSObject properties visible inside code generators.  I will advocate that doing so is safe as long as the CodeGenerators do not aggregate or manipulate JSObject values.
Comment 10 Nicolas B. Pierron [:nbp] 2011-10-07 07:29:57 PDT
(In reply to David Anderson [:dvander] from comment #8)
> Comment on attachment 564242 [details] [diff] [review] [diff] [details] [review]
> [0001] Add number of registers used to ship boxed values.
> 
> Review of attachment 564242 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/x86/Assembler-x86.h
> @@ +69,4 @@
> >  static const Register JSCReturnReg_Type = eax;
> >  static const Register JSCReturnReg_Data = edx;
> >  static const FloatRegister ScratchFloatReg = { JSC::X86Registers::xmm7 };
> > +static const uint32 NumJSReg = 2;
> 
> This name is kind of mysterious - how about NumRegsPerValue? (or
> NumValueRegs)?

Indeed, I'll apply the change.

I wonder why do we review+ even with blocking comments.  Could we be more strict with the review+ because having blocking comments and a positive review sends an ambiguous message.
Comment 11 Nicolas B. Pierron [:nbp] 2011-11-11 17:36:21 PST
Created attachment 573974 [details] [diff] [review]
[0004] Add prototype-policy: use a complex prototype for the type policy of MIR Instructions.
Comment 12 Nicolas B. Pierron [:nbp] 2011-11-11 17:37:10 PST
Created attachment 573975 [details] [diff] [review]
[0005] Add optimized version of JSOP_INITELEM.
Comment 13 David Anderson [:dvander] 2011-11-11 23:10:05 PST
Looking at BytecodeEmitter.cpp and JM+TI, it seems like JSOP_INITELEM always has a constant, numeric index. I think we discussed this a bit IRL, but hadn't come to a conclusion yet. Given that, I think the ideal thing here is to choose two ways to compile INITELEM, and to make this decision in IonBuilder:

 (1) If the TypeOracle tells us we can't specialize the array, we can just abort for now.
     (In the future, we'll want this to become a slow-path, but let's keep it simple to
      start.)
 (2) If the TypeOracle tells us we *can* specialize the array, emit a series of MIR ops:
     v0 = MLoadSlots(array)
     v1 = MStoreElement(v0, index, value)
     --   MUpdateLength(array)

StoreElement can be modeled off Jan's work for StoreSlot in bug 700303.
UpdateLength is needed if TI is enabled - this decision should be hidden behind the Oracle, though.

I think this simplifies things while providing us with a bunch of reusable components - for example, UpdateLength can be used for array.push and StoreElement for SETELEM.
Comment 14 David Anderson [:dvander] 2011-11-11 23:12:31 PST
(Err - well maybe UpdateLength can't - but StoreElement, definitely.)
Comment 15 Nicolas B. Pierron [:nbp] 2011-12-30 17:43:50 PST
Created attachment 585089 [details] [diff] [review]
Implement JSOP_INITELEM.
Comment 16 Chris Leary [:cdleary] (not checking bugmail) 2012-02-03 04:04:17 PST
Bump, what's the status on this op? It's causing aborted compiles in SunSpider. I can run the patch to the finish line if need be.
Comment 17 Nicolas B. Pierron [:nbp] 2012-02-03 04:20:24 PST
I still have the patch locally and it should be easy to rebase.  I just need a review to land it.

Chris, if you want to review it, feel free ;)
Comment 18 Chris Leary [:cdleary] (not checking bugmail) 2012-02-03 04:25:12 PST
Oh okay great -- do you mind rebasing it? It may have just fallen by the wayside because of assumed bitrot.
Comment 19 Nicolas B. Pierron [:nbp] 2012-02-03 05:19:30 PST
The patch is rebased localy, It is identical except for ARM store32 & storePtr impl since they already exists.
Comment 20 Jan de Mooij [:jandem] 2012-02-08 04:07:03 PST
Comment on attachment 585089 [details] [diff] [review]
Implement JSOP_INITELEM.

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

Nice work, r=me with the comments addressed.

(Stealing review since this blocks some benchmarks.)

::: js/src/ion/CodeGenerator.cpp
@@ +551,5 @@
> +    } else {
> +        Register l = ToRegister(length);
> +        masm.inc32(l);
> +        masm.store32(l, initLength);
> +    }

Nit: You can use ToInt32Key and bumpKey/branchKey here nowadays, see OutOfLineStoreElementHole.

It's also probably best to restore the original length value (bumpKey(-1), even though it shouldn't matter for initelem I think).

::: js/src/ion/IonBuilder.cpp
@@ +2315,5 @@
> +    MDefinition *obj = current->peek(-1);
> +
> +    // Get the elements vector.
> +    MElements *elements = MElements::New(obj);
> +    current->add(elements);

Looking at JM, the object is not necessarily a dense array. Can you add an abort() if obj->isNewArray() is false and file a follow-up bug to change it to an InitElement stub call?

@@ +2324,5 @@
> +
> +    // Update the length if this initelem is the last one.
> +    if (JSOp(*GetNextPc(pc)) == JSOP_ENDINIT) {
> +        MSetInitializedLength *initLength = MSetInitializedLength::New(elements, id);
> +        current->add(initLength);

I think we'll have to update the length after every INITELEM. If the array literal is [{}, f()], and f() triggers a GC, we want the GC to see the first value.

Since the store and length update are both effectful, also add a "return resumeAfter(initLength)"
Comment 21 David Anderson [:dvander] 2012-02-08 14:37:13 PST
Sorry, had to back this out since kraken is crashing - I've narrowed it down to this test:

./sunspider --runs=1 \
            --args="--ion -n" \
            --shell=./IM \
            --suite=kraken-1.1 \
            --test=stanford-crypto-ccm

Call stack:
#0  getParent (this=<optimized out>) at /home/dvander/mozilla/work-ion/js/src/jsarray.cpp:3865
#1  global (this=<optimized out>) at ../jsobjinlines.h:1424
#2  js::GetCurrentGlobal (cx=<optimized out>) at ../jsobjinlines.h:1764
#3  0x08071772 in NewArray<true> (proto=NULL, length=0, cx=0x90ae4c8) at /home/dvander/mozilla/work-ion/js/src/jsarray.cpp:3746
#4  js::NewDenseAllocatedArray (cx=0x90ae4c8, length=0, proto=NULL) at /home/dvander/mozilla/work-ion/js/src/jsarray.cpp:3808
#5  0x080c07de in js::NewInitArray (cx=0x90ae4c8, count=0, type=0xf6c2a260) at /home/dvander/mozilla/work-ion/js/src/jsinterp.cpp:4642
#6  0x080c4ae5 in js::Interpret (cx=0x90ae4c8, entryFrame=0xf6e2d788, interpMode=js::JSINTERP_BAILOUT) at /home/dvander/mozilla/work-ion/js/src/jsinterp.cpp:3710
#7  0x082c889a in js::ion::ThunkToInterpreter (vp=0xff8748dc) at /home/dvander/mozilla/work-ion/js/src/ion/Bailouts.cpp:501

http://hg.mozilla.org/projects/ionmonkey/rev/2e4b0fad5914
Comment 22 Nicolas B. Pierron [:nbp] 2012-02-21 22:11:45 PST
https://hg.mozilla.org/projects/ionmonkey/rev/acb08144edf1

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