Closed
Bug 710130
Opened 12 years ago
Closed 12 years ago
IonMonkey: Implement callVM on ARM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: nbp)
References
Details
Attachments
(1 file)
46.25 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
This is necessary to move NewArray to CodeGenerator.cpp, as well as other incoming opcode implementations like GETPROP and bhackett's GETPROP ICs.
Assignee | ||
Updated•12 years ago
|
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
Update generateVMWrapper and factor callVM inside CGShared.
Attachment #583700 -
Flags: review?(dvander)
Reporter | ||
Comment 2•12 years ago
|
||
Comment on attachment 583700 [details] [diff] [review] callVM ARM Review of attachment 583700 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits picked ::: js/src/ion/arm/Trampoline-arm.cpp @@ +501,3 @@ > MacroAssembler masm; > + GeneralRegisterSet regs = > + GeneralRegisterSet::Not(GeneralRegisterSet(Register::Codes::VolatileMask)); What's the significance of choosing ~Volatile here? ::: js/src/ion/shared/CodeGenerator-shared.cpp @@ +290,5 @@ > + return true; > +} > + > +bool > +CodeGeneratorShared::visitNewArray(LNewArray *ins) visitNewArray and visitLoadPropertyGeneric should be in CodeGenerator, rather than CodeGenerator-shared. CG-shared is intended for helper functions that are usable across all architectures, whereas CG is the "frontend". ::: js/src/ion/shared/CodeGenerator-shared.h @@ +66,5 @@ > LIRGraph &graph; > LBlock *current; > SnapshotWriter snapshots_; > IonCode *deoptTable_; > +#ifdef DEBUG Newline above DEBUG (though I think it looks nicer without the DEBUG). @@ +183,4 @@ > template <typename T> > void pushArg(const T &t) { > masm.Push(t); > +#ifdef DEBUG Ditto. ::: js/src/ion/shared/MacroAssembler-x86-shared.h @@ +141,5 @@ > } > void load32(const Address &address, Register dest) { > movl(Operand(address), dest); > } > + void callExitIon(IonCode *target) { Please rename this to "callWithDescriptor" ::: js/src/ion/x86/Trampoline-x86.cpp @@ +480,5 @@ > > // Initialize and set the context parameter. > Register cxreg = regs.takeAny(); > + masm.movePtr(ImmWord(JS_THREAD_DATA(cx)), cxreg); > + masm.loadPtr(Address(cxreg, offsetof(ThreadData, ionJSContext)), cxreg); What was the reasoning behind this change?
Attachment #583700 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 583700 [details] [diff] [review] callVM ARM Review of attachment 583700 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/arm/Trampoline-arm.cpp @@ +501,3 @@ > MacroAssembler masm; > + GeneralRegisterSet regs = > + GeneralRegisterSet::Not(GeneralRegisterSet(Register::Codes::VolatileMask)); On ARM, Volatile registers are registers used for arguments using the same set of registers as setABIArg cause issues in the MoveEmitter. I would have prefered to use Volatile registers here to later be able to keep non-GC values into registers across calls, but we will see when we implement such optimization. ::: js/src/ion/shared/CodeGenerator-shared.h @@ +66,5 @@ > LIRGraph &graph; > LBlock *current; > SnapshotWriter snapshots_; > IonCode *deoptTable_; > +#ifdef DEBUG pushedArgs_ is only used in a JS_ASSERT in callVM function. So I don't see any need to keep it updating it if it is never used in optimized builds. ::: js/src/ion/shared/MacroAssembler-x86-shared.h @@ +141,5 @@ > } > void load32(const Address &address, Register dest) { > movl(Operand(address), dest); > } > + void callExitIon(IonCode *target) { Unfortunately not. The reason is that ARM may end-up having different frame layout for the exit frame and other JS frames. What I can do is callIonFrame(FrameType current, FrameType called) ::: js/src/ion/x86/Trampoline-x86.cpp @@ +480,5 @@ > > // Initialize and set the context parameter. > Register cxreg = regs.takeAny(); > + masm.movePtr(ImmWord(JS_THREAD_DATA(cx)), cxreg); > + masm.loadPtr(Address(cxreg, offsetof(ThreadData, ionJSContext)), cxreg); to be consistent with the ARM implementation which does not have an equivalent of masm.movl(Operand(…), reg). and we don't have Address(offset) constructor.
Reporter | ||
Comment 5•12 years ago
|
||
> ::: js/src/ion/shared/MacroAssembler-x86-shared.h
> @@ +141,5 @@
> > }
> > void load32(const Address &address, Register dest) {
> > movl(Operand(address), dest);
> > }
> > + void callExitIon(IonCode *target) {
>
> Unfortunately not. The reason is that ARM may end-up having different frame
> layout for the exit frame and other JS frames.
>
> What I can do is callIonFrame(FrameType current, FrameType called)
Okay, "callWithExitFrame" then.
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/60766fb64a78
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•