+++ This bug was initially created as a clone of Bug #685838 +++ Implement the opcode JSOP_INITELEM in ionmonkey.

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 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 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)?

(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.

(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.

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.

(Err - well maybe UpdateLength can't - but StoreElement, definitely.)

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.

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 ;)

Oh okay great -- do you mind rebasing it? It may have just fallen by the wayside because of assumed bitrot.

The patch is rebased localy, It is identical except for ARM store32 & storePtr impl since they already exists.

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)"

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