Closed
Bug 695897
Opened 13 years ago
Closed 13 years ago
IonMonkey: Refactor IonFrames and Bailouts
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(7 files, 1 obsolete file)
32.49 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
30.45 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
14.94 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
59.94 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
20.33 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
20.25 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
11.19 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
This work is to make way for proper exception handling and GC'ing while Ion code is on the stack. The plan is to give ion::ThunkToInterpreter a proper exit frame so it looks like a normal call to C++. This will also introduce some cleaner constructors for frames and bailouts.
Split off from bug 695075.
Assignee | ||
Comment 1•13 years ago
|
||
(In reply to Nicolas B. Pierron [:pierron] from bug 695075 comment #4)
> > +class IonEntryFrameLayout
> > +{
>
> class IonEntryFrameLayout : public IonCommonFrameLayout
Whoops, thanks.
> > + double fpregs_[FloatRegisters::Total];
> > + uintptr_t regs_[Registers::Total];
>
> I would prefer to have a name for this couple, such as ProcessContext /
> RegistersContext / MachineState and provide a RegistersContextRef /
> MachineStateRef class in place of the current MachineState.
>
> Thus we could have the corresponding macro assembler instructions for making
> context.
I like the idea, though we don't have a clear definition for a context yet. For example, in some situations, we may only want to save double registers. but:
> > JS_STATIC_ASSERT(sizeof(BailoutStack) ==
> > sizeof(uintptr_t) +
> > sizeof(double) * 8 +
> > sizeof(uintptr_t) * 8 +
>
> 8 is a nice inlining of Registers::Total & FloatRegisters::Total values. ;)
> sizeof(RegistersContext) is a good substitute.
Hrm - yeah, this is gross. We should really be using NumAllocatableRegisters and only pushing registers in that set. I'll clean this up.
> 1: Thus I wonder why you reinterpret data in a larger type than what it can
> be. Instead of a void **esp, you should expect a "BailoutStack *". This
> would make the prototype of the function cleaner for readers.
Good idea.
Assignee | ||
Comment 2•13 years ago
|
||
This patch changes implicit Value outparams to be explicit
Attachment #571869 -
Flags: review?(sstangl)
Assignee | ||
Comment 3•13 years ago
|
||
This moves ABI calls to be specific to x86/x64 macro assemblers. ARM will come next. This makes it possible to perform ABI calls from within arch-specific assemblers.
Attachment #571870 -
Flags: review?(sstangl)
Assignee | ||
Comment 4•13 years ago
|
||
Make ABI class arch-specific, ARM changes.
Attachment #572063 -
Flags: review?(mrosenberg)
Comment 5•13 years ago
|
||
Comment on attachment 572063 [details] [diff] [review]
part 3: ARM changes
Review of attachment 572063 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/arm/MacroAssembler-arm.h
@@ +332,5 @@
> void breakpoint();
>
> + // Setup a call to C/C++ code, given the number of general arguments it
> + // takes. Note that this only supports cdecl.
> + //
As Jacob Bramley has pointed out in the past, cdecl doesn't exist on arm. The whole ABI is called "eabi", which is probably good enough to distinguish the calling convention
@@ +353,5 @@
> + void setABIArg(uint32 arg, const MoveOperand &from);
> + void setABIArg(uint32 arg, const Register ®);
> +
> + // Emits a call to a C/C++ function, resolving all argument moves.
> + void callWithABI(void *fun);
I'd actually put all of this code into Compat. My idea was that eventually, we can mark the entire body of MacroAssemblerARM as protected, and have all code that isn't in the arm/ directory to only have access to the compat assembler, not the underlying assembler. However, I don't know if this is a good direction for the code to head in
::: js/src/ion/arm/MoveEmitter-arm.cpp
@@ +48,1 @@
> : inCycle_(false),
In general, everything in the arm directory should be able to suffice using MacroAssemblerARM rather than MacroAssemblerARMCompat, but I don't want to unnecessarily restrict future code.
Attachment #572063 -
Flags: review?(mrosenberg) → review+
Assignee | ||
Comment 6•13 years ago
|
||
untested, builds on x86. this patch introduces a number of refactorings:
* Frames are now opaque, hidden behind a new IonFrameIterator, which must start
iterating from a pointer to an exit frame.
* When leaving JIT code to potentially re-enter the VM, an exit frame must be
created and a pointer to its header stored in ThreadData.
* returnError trampolines are now gone. Instead, calls within an exit frame
must check for failure and then call an exception handler. The exception
handler returns a new stack pointer.
* BailoutEnvironment has been refactored into FrameRecovery and MachineState,
which can be shared across architectures.
* Architecture-specific bailout code is now much simpler, you just need to
supply a MachineState and FrameRecovery.
* Bailouts now replace the deoptimized JS frame with an Exit frame, and call
the interpreter from that (as if it were called from generateCWrapper).
Assignee | ||
Comment 7•13 years ago
|
||
x86 changes - x64 and ARM in the next two patches
Attachment #572381 -
Flags: review?(sstangl)
Assignee | ||
Updated•13 years ago
|
Attachment #572153 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #572396 -
Flags: review?(sstangl)
Assignee | ||
Comment 9•13 years ago
|
||
This patch changes ABI calls to use the same ExitFrame mechanism as bailouts. For the most part this just simplifies things.
Attachment #572561 -
Flags: review?(sstangl)
Assignee | ||
Comment 10•13 years ago
|
||
Hopefully the last patch. This one makes Ion re-entrant again, by saving/restoring ionTop. It also makes ABI calls no longer bake in JSContext, which isn't safe (yet).
Attachment #572711 -
Flags: review?(sstangl)
Comment 11•13 years ago
|
||
Comment on attachment 571869 [details] [diff] [review]
part 1: use explicit outparams in ABI calls
Review of attachment 571869 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/IonMacroAssembler.cpp
@@ +105,5 @@
> {
> JS_ASSERT(inCall_);
>
> + // Position all arguments.
> + {
The extra indentation level can be removed.
@@ +110,3 @@
> enoughMemory_ &= moveResolver_.resolve();
> if (!enoughMemory_)
> return;
If we don't have enough memory to perform necessary moves, then the result is nonsense. Should callWithABI() be made fallible?
::: js/src/ion/x64/Trampoline-x64.cpp
@@ +436,5 @@
> + Register outReg = InvalidReg;
> + if (f.outParam == VMFunction::OutParam_Value) {
> + outReg = regs.takeAny();
> + masm.reserveStack(sizeof(Value));
> + masm.movl(rsp, outReg);
Great!
@@ +445,2 @@
>
> + // Set the implicit context parameter.
nit: // Initialize and set the implicit context parameter.
Then we can eliminate the latter comment and the whitespace for no less clarity.
@@ +463,5 @@
>
> + // Load the outparam and free any allocated stack.
> + if (f.outParam == VMFunction::OutParam_Value) {
> + masm.loadValue(Operand(esp, 0), JSReturnOperand);
> + masm.freeStack(sizeof(Value));
=]
::: js/src/ion/x86/Assembler-x86.h
@@ -66,5 @@
> static const Register JSReturnReg_Data = edx;
> static const Register StackPointer = esp;
> static const Register ReturnReg = eax;
> -static const Register JSCReturnReg_Type = eax;
> -static const Register JSCReturnReg_Data = edx;
JSCReturnReg_Type, JSCReturnReg_Data, and JSCReturnOperand also exist in Assembler-x64.h. Since this patch removes both uses of JSCReturnOperand in Trampoline-x64.cpp, these definitions can be removed for x64 as well.
Attachment #571869 -
Flags: review?(sstangl) → review+
Comment 12•13 years ago
|
||
Comment on attachment 571870 [details] [diff] [review]
part 2: make ABI calls arch-specific
Review of attachment 571870 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
nit: shouldn't tab-width be '4'? This looks like it affects a bunch of files.
Attachment #571870 -
Flags: review?(sstangl) → review+
Comment 13•13 years ago
|
||
Comment on attachment 572381 [details] [diff] [review]
part 4: introduce exit frames for bailouts, refactoring
Review of attachment 572381 [details] [diff] [review]:
-----------------------------------------------------------------
This patch is /extremely/ pleasant. A bunch of annoying nits follow.
::: js/src/ion/IonFrames.cpp
@@ +48,5 @@
> +using namespace js;
> +using namespace js::ion;
> +
> +FrameRecovery::FrameRecovery(uint8 *fp, uint8 *sp, const MachineState &machine)
> + : fp_((IonJSFrameLayout *)fp),
Since IonJSFrameLayouts have certain known types, can we assert in the body below that fp actually points to an IonJSFrameLayout?
::: js/src/ion/IonFrames.h
@@ +175,5 @@
> + MachineState(uintptr_t *regs, double *fpregs)
> + : regs_(regs), fpregs_(fpregs)
> + { }
> +
> + double readDoubleReg(FloatRegister reg) const {
readFloatReg()? They're called FloatRegisters everywhere else. Maybe they should be called something different.
@@ +223,5 @@
> + uintptr_t readSlot(uint32 offset) const {
> + JS_ASSERT((offset % STACK_SLOT_SIZE) == 0);
> + return *(uintptr_t *)(sp_ + offset);
> + }
> + double readDoubleSlot(uint32 offset) const {
readFloatSlot()? Or rename FloatRegister, as above.
@@ +287,5 @@
> return (JSScript*)(uintptr_t(token) & ~uintptr_t(1));
> }
>
> }
> }
// namespace ion
// namespace js
::: js/src/ion/shared/IonFrames-x86-shared.h
@@ +63,5 @@
> + return descriptor_ >> FRAMETYPE_BITS;
> + }
> +};
> +
> +class IonEntryFrameLayout
This appears to be empty. Perhaps a relic of refactoring? Certainly it should at least extend IonCommonFrameLayout.
@@ +68,5 @@
> +{
> + private:
> +};
> +
> +class IonJSFrameLayout : public IonCommonFrameLayout
It would be useful to occasionally assert that, based on the descriptor, the right class is mapped to the right stack location.
@@ +78,5 @@
> + void *calleeToken() const {
> + return calleeToken_;
> + }
> +
> + static size_t offsetOfCalleeToken() {
nit: static functions typically start with a capital.
@@ +101,5 @@
> uintptr_t snapshotOffset;
> };
>
> }
> }
// namespace ion
// namespace js
::: js/src/ion/x86/Architecture-x86.h
@@ +83,5 @@
> static const Code StackPointer = JSC::X86Registers::esp;
> static const Code Invalid = JSC::X86Registers::invalid_reg;
>
> static const uint32 Total = 8;
> + static const uint32 Allocatable = 7;
Oh dear.
::: js/src/ion/x86/Assembler-x86.h
@@ +237,5 @@
> }
> + void movl(ImmWord imm, Register dest) {
> + masm.movl_i32r(imm.value, dest.code());
> + }
> + void mov(Imm32 imm, Register dest) {
We should probably standardize on either "Register dest" or "const Register &dest", since in practice it makes no difference, but having multiple styles is annoying. I prefer the former.
::: js/src/ion/x86/Bailouts-x86.cpp
@@ +107,3 @@
>
> + if (bailout->frameClass() == FrameSizeClass::None())
> + return FrameRecovery::FromSnapshot(fp, sp, bailout->machine(), bailout->snapshotOffset());
This is exceptionally pretty.
::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +144,5 @@
> inCall_ = false;
> }
>
> +void
> +MacroAssemblerX86::handleException()
This is pleasant.
::: js/src/ion/x86/Trampoline-x86.cpp
@@ +123,2 @@
>
> // Save the stack size so we can remove arguments and alignment after the
Above this line, the callee token is still pushed. But IonEntryFrameLayout doesn't have a calleeToken. One of these is wrong: I suspect IonEntryFrameLayout.
@@ +238,5 @@
>
> // Construct sizeDescriptor.
> masm.subl(esp, ebp);
> + masm.shll(Imm32(FRAMETYPE_BITS), ebp);
> + masm.orl(Imm32(IonFrame_Rectifier), ebp);
Should we make constructing a sizeDescriptor a function in the MacroAssembler? It might be nice to not have the logic splayed out in many locations.
@@ +254,5 @@
> masm.call(eax);
>
> // Remove the rectifier frame.
> masm.pop(ebp); // ebp <- sizeDescriptor with FrameType.
> + masm.shrl(Imm32(FRAMETYPE_BITS), ebp); // ebp <- sizeDescriptor.
Just "descriptor" now, I believe =]
@@ +281,5 @@
> // Push the bailout table number.
> masm.push(Imm32(frameClass));
>
> + // Get the stack pointer into a register. This is the first argument to
> + // ion:Bailout.
nit: "// Stack pointer is the first argument to ion::Bailout()."
@@ +290,5 @@
> masm.setABIArg(0, eax);
> masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, Bailout));
>
> + // Common size of all the stuff we've pushed since creating the external
> + // frame.
nit: -we've +frame <== fits on one line.
::: js/src/jscntxt.h
@@ +216,5 @@
> PendingProxyOperation *pendingProxyOperation;
>
> ConservativeGCThreadData conservativeGC;
>
> + uint8 *ionTop;
Comment on usage? It is not obvious that there is only one ionTop, given the possibility of nested ion/interp/ion/interp frames.
Attachment #572381 -
Flags: review?(sstangl) → review+
Comment 14•13 years ago
|
||
Comment on attachment 572396 [details] [diff] [review]
part 5: x64 changes
Review of attachment 572396 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/x64/Bailouts-x64.cpp
@@ +46,5 @@
>
> using namespace js;
> using namespace js::ion;
>
> +#if defined(_WIN32)
_WIN64?
@@ +78,5 @@
> +
> +} // namespace ion
> +} // namespace js
> +
> +#if defined(_WIN32)
_WIN64?
::: js/src/ion/x64/Trampoline-x64.cpp
@@ +134,5 @@
> Push the number of bytes we've pushed so far on the stack and call
> *****************************************************************/
> masm.subq(rsp, r14);
> + masm.shlq(Imm32(FRAMETYPE_BITS), r14);
> + masm.orl(Imm32(IonFrame_Entry), r14);
masm.makeDescriptor(r14, IonFrame_Entry)?
@@ +282,5 @@
> masm.reserveStack(FloatRegisters::Total * sizeof(double));
> for (uint32 i = 0; i < FloatRegisters::Total; i++)
> masm.movsd(FloatRegister::FromCode(i), Operand(rsp, i * sizeof(double)));
>
> + masm.breakpoint();
Trace/breakpoint trap (core dumped)
@@ +333,5 @@
> + // Store to ThreadData::ionTop. Note that rax still holds the return value
> + // from ion::Bailout.
> +
> + masm.movq(ImmWord(JS_THREAD_DATA(cx)), rdx);
> + masm.movq(rsp, Operand(rdx, offsetof(ThreadData, ionTop)));
Is (JS_THREAD_DATA(cx) + offsetof(ThreadData, ionTop)) is already a compile-time constant? We could move it directly into %rdx. (Likewise for x86.)
Attachment #572396 -
Flags: review?(sstangl) → review+
Comment 15•13 years ago
|
||
Comment on attachment 572561 [details] [diff] [review]
part 6: make ABI calls use ExitFrames
Review of attachment 572561 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/shared/MacroAssembler-x86-shared.h
@@ +121,5 @@
> + // Save an exit frame (which must be aligned to the stack pointer) to
> + // ThreadData::ionTop.
> + void linkExitFrame(Register scratch) {
> + mov(ImmWord(JS_THREAD_DATA(GetIonContext()->cx)), scratch);
> + mov(StackPointer, Operand(scratch, offsetof(ThreadData, ionTop)));
This can be made more efficient without the use of a scratch register:
"JS_THREAD_DATA(...) + offsetof(ThreadData, ionTop)" is a compile-time constant.
::: js/src/ion/x64/Assembler-x64.h
@@ +370,5 @@
> JS_NOT_REACHED("unexpected operand kind");
> }
> }
>
> + void mov(ImmWord word, const Register &dest) {
"const ImmWord &word"? We should canonicalize on a passing style.
Attachment #572561 -
Flags: review?(sstangl) → review+
Comment 16•13 years ago
|
||
(In reply to Sean Stangl from comment #15)
> ::: js/src/ion/shared/MacroAssembler-x86-shared.h
> @@ +121,5 @@
> > + // Save an exit frame (which must be aligned to the stack pointer) to
> > + // ThreadData::ionTop.
> > + void linkExitFrame(Register scratch) {
> > + mov(ImmWord(JS_THREAD_DATA(GetIonContext()->cx)), scratch);
> > + mov(StackPointer, Operand(scratch, offsetof(ThreadData, ionTop)));
>
> This can be made more efficient without the use of a scratch register:
> "JS_THREAD_DATA(...) + offsetof(ThreadData, ionTop)" is a compile-time
> constant.
Oops, I missed that this was in x86-shared. It can be written with a single move on x86, whereas x64 already has the ScratchReg.
Comment 17•13 years ago
|
||
Comment on attachment 572711 [details] [diff] [review]
part 7: properly stack exit frames
Review of attachment 572711 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/x64/MacroAssembler-x64.h
@@ +448,5 @@
> + // Save an exit frame (which must be aligned to the stack pointer) to
> + // ThreadData::ionTop.
> + void linkExitFrame(Register scratch) {
> + mov(ImmWord(JS_THREAD_DATA(GetIonContext()->cx)), scratch);
> + mov(StackPointer, Operand(scratch, offsetof(ThreadData, ionTop)));
Can be rewritten without a specific "scratch" register -- x86 doesn't need one, and x64 already has ScratchReg. Also the offsetof() can be moved into the first mov.
::: js/src/ion/x64/Trampoline-x64.cpp
@@ +426,5 @@
> masm.setupUnalignedABICall(f.argc(), temp);
>
> // Initialize the context.
> + masm.movq(ImmWord(JS_THREAD_DATA(cx)), ArgReg0);
> + masm.movq(Operand(ArgReg0, offsetof(ThreadData, ionTop)), ArgReg0);
ionJSContext
masm.movq(ImmWord(((uint8*)JS_THREAD_DATA(cx)) + offestof(ThreadData, ionTop)), ScratchReg);
masm.movq(Operand(ScratchReg), ArgReg0);
I would expect this to be faster than the alternative.
::: js/src/ion/x86/MacroAssembler-x86.h
@@ +397,5 @@
> void handleException();
> +
> + // Save an exit frame (which must be aligned to the stack pointer) to
> + // ThreadData::ionTop.
> + void linkExitFrame(Register scratch) {
Scratch register is unused.
::: js/src/ion/x86/Trampoline-x86.cpp
@@ +445,5 @@
> masm.setupUnalignedABICall(f.argc(), temp);
>
> // Initialize the context.
> Register cxreg = regs.takeAny();
> + masm.movl(Operand(&JS_THREAD_DATA(cx)->ionTop), cxreg);
ionJSContext
Attachment #572711 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 18•13 years ago
|
||
> If we don't have enough memory to perform necessary moves, then the result
> is nonsense. Should callWithABI() be made fallible?
This sets enoughMemory=false, which the linker will see when it checks MacroAssembler::oom().
> > +FrameRecovery::FrameRecovery(uint8 *fp, uint8 *sp, const MachineState &machine)
> > + : fp_((IonJSFrameLayout *)fp),
>
> Since IonJSFrameLayouts have certain known types, can we assert in the body
> below that fp actually points to an IonJSFrameLayout?
Hrm, I think we can only assert on the previous frame type, not the current one.
> readFloatReg()? They're called FloatRegisters everywhere else. Maybe they
> should be called something different.
readFloatReg() is fine. I had used "Double" for parity with readDoubleSlot().
> readFloatSlot()? Or rename FloatRegister, as above.
Erg. I'll save this for another day. I think DoubleRegister would be best for clarity. "FloatRegister" isn't meant to mean "32-bit float" but "Floating-Point Register".
> nit: static functions typically start with a capital.
Luke and njn have been making exceptions for the offsetOf ones, so they look sort of like the macro.
> We should probably standardize on either "Register dest" or "const Register
> &dest", since in practice it makes no difference, but having multiple styles
> is annoying. I prefer the former.
Yeah, "Register dest" is way better. Let's just delete the const and & incrementally.
> > // Save the stack size so we can remove arguments and alignment after the
>
> Above this line, the callee token is still pushed. But IonEntryFrameLayout
> doesn't have a calleeToken.
The callee is pushed as part of the outgoing JS frame - the "entry frame" really doesn't exist, I think. It's just a stopping point. I'll delete the class if it's totally unused, I think it is.
> Should we make constructing a sizeDescriptor a function in the
> MacroAssembler? It might be nice to not have the logic splayed out in many
> locations.
Good idea - done.
Assignee | ||
Comment 19•13 years ago
|
||
> > +#if defined(_WIN32)
>
> _WIN64?
_WIN32 is set on _WIN64 as well
> > + masm.breakpoint();
>
> Trace/breakpoint trap (core dumped)
You can tell I do lots of testing :)
> Is (JS_THREAD_DATA(cx) + offsetof(ThreadData, ionTop)) is already a
> compile-time constant? We could move it directly into %rdx. (Likewise for
> x86.)
Yeah it is, I like having the pointer though. It's easier to debug.
> Can be rewritten without a specific "scratch" register
Done.
> > // Initialize the context.
> > + masm.movq(ImmWord(JS_THREAD_DATA(cx)), ArgReg0);
> > + masm.movq(Operand(ArgReg0, offsetof(ThreadData, ionTop)), ArgReg0);
>
> ionJSContext
Whoops, good catch.
Assignee | ||
Comment 20•13 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/8284e6365e99
http://hg.mozilla.org/projects/ionmonkey/rev/27dda98f1ece
http://hg.mozilla.org/projects/ionmonkey/rev/b54f9c27c458
http://hg.mozilla.org/projects/ionmonkey/rev/6aefeb68c04d
http://hg.mozilla.org/projects/ionmonkey/rev/6778cd6941a1
http://hg.mozilla.org/projects/ionmonkey/rev/2251703b072e
http://hg.mozilla.org/projects/ionmonkey/rev/29c5f1b61413
Filed bug 701984 for ARM
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•