Closed
Bug 685228
Opened 13 years ago
Closed 13 years ago
IonMonkey: setABIArg should use MoveOperand instead of Register.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(1 file, 1 obsolete file)
5.87 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
Function setABIArg and callWithABI are using a MoveResolver which is resolved inside callWithABI. Not using MoveOperand as argument of setABIArg can caused trouble if multiple arguments are used. A simple example is: masm.setABIArg(0, rax); masm.mov(Operand(rbx, 0), rax); masm.setABIArg(1, rax); Due to the resolve, not being aware of intermediate instructions, the previous code produce the following assembly: mov (%rbx), %rax ; mov(Operand(rbx, 0), rax) mov %rax, %rsi ; setABIArg(0, rax) mov %rax, %rdi ; setABIArg(1, rax) Using MoveOperand instead of Register as argument of setABIArg does not remove the problem of intermediate instructions but should solve the ordering problem.
Assignee | ||
Updated•13 years ago
|
Assignee: general → npierron
Assignee | ||
Comment 1•13 years ago
|
||
This patch is necessary to handle calls with large argument list with C++ calls. (see Bug 680315). It does not yet remove the Register version of setABIArg.
Attachment #559451 -
Flags: review?(sstangl)
Assignee | ||
Comment 2•13 years ago
|
||
(In reply to Nicolas B. Pierron [:pierron] from comment #1) > Created attachment 559451 [details] [diff] [review] > Add setABIArg function with MoveOperand argument. (Quote from IRC) > 19:18:11 <dvander> what if we added a setABIArg(n, Address) ? > 19:18:18 <pierron> I did > 19:18:29 <pierron> using MoveOperand > 19:18:58 <pierron> Such way, I succeed to call a function with 11 arguments. > 19:19:16 <dvander> ah, ok - i think that's the right idea, but MoveOperand is probably not the struct we want > 19:19:25 <dvander> it's supposed to be internal to MoveResolver > ... > 19:20:58 <dvander> Address is intended to be a cross-platform version of Operand Bug 684402 introduces Address(base, offset) struct.
Depends on: 684402
Comment 3•13 years ago
|
||
Comment on attachment 559451 [details] [diff] [review] Add setABIArg function with MoveOperand argument. Review of attachment 559451 [details] [diff] [review]: ----------------------------------------------------------------- Sorry this took so long; it totally fell off my radar. I just learned about the "My Requests" link in Bugzilla, so that won't happen again. r+ with nits addressed, GetArgStackSlot() returning (uint32 *), and an ARM implementation up. ::: js/src/ion/IonMacroAssembler.cpp @@ +87,2 @@ > Register dest; > + if (GetArgReg(arg, &dest)) if (GetArgReg(arg, &dest)) { @@ +87,5 @@ > Register dest; > + if (GetArgReg(arg, &dest)) > + to = MoveOperand(dest); > + else > + { } else { @@ +92,4 @@ > // There is no register for this argument, so just move it to its > // stack slot immediately. > + uint32 disp; > + GetArgStackSlot(arg, &disp); Either GetArgStackSlot() shouldn't return bool, or this function needs to check for error. The x64 implementation return false occasionally, in which case this function passes uninitialized memory to MoveOperand(). Maybe talk with mjrosenb to get an ARM implementation up -- it should be simple. Then we don't have to return bool for a very simple calculation. ::: js/src/ion/arm/Assembler-arm.h @@ +1024,5 @@ > +{ > + if (arg != 0) > + return false; > + JS_NOT_REACHED("Codegen for GetArgStackSlot NYI"); > +#if 0 This block shouldn't exist. This function needs a "return false;" after the NYI assertion, to handle opt builds. ::: js/src/ion/x64/Assembler-x64.h @@ +497,5 @@ > +static inline bool > +GetArgStackSlot(uint32 arg, uint32 *disp) > +{ > + if (arg < NumArgRegs) > + return false; This looks like it should become an assertion. Returning here doesn't really make sense. ::: js/src/ion/x86/Assembler-x86.h @@ +337,5 @@ > return false; > } > > +static inline bool > +GetArgStackSlot(uint32 arg, uint32 *disp) This doesn't really get a stack slot; it gets a displacement.
Attachment #559451 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 4•13 years ago
|
||
Update (old patch, r=sstangl)
Attachment #559451 -
Attachment is obsolete: true
Attachment #567791 -
Flags: review+
https://hg.mozilla.org/projects/ionmonkey/rev/ae2ac74f31ae
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•