Last Comment Bug 685228 - IonMonkey: setABIArg should use MoveOperand instead of Register.
: IonMonkey: setABIArg should use MoveOperand instead of Register.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on: 684402
Blocks: 680315 692114
  Show dependency treegraph
 
Reported: 2011-09-07 10:58 PDT by Nicolas B. Pierron [:nbp]
Modified: 2011-10-20 13:39 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add setABIArg function with MoveOperand argument. (6.08 KB, patch)
2011-09-09 07:26 PDT, Nicolas B. Pierron [:nbp]
sstangl: review+
Details | Diff | Review
Add setABIArg function with MoveOperand argument. (5.87 KB, patch)
2011-10-18 10:35 PDT, Nicolas B. Pierron [:nbp]
nicolas.b.pierron: review+
Details | Diff | Review

Description Nicolas B. Pierron [:nbp] 2011-09-07 10:58:07 PDT
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.
Comment 1 Nicolas B. Pierron [:nbp] 2011-09-09 07:26:51 PDT
Created attachment 559451 [details] [diff] [review]
Add setABIArg function with MoveOperand argument.

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.
Comment 2 Nicolas B. Pierron [:nbp] 2011-09-09 11:30:35 PDT
(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.
Comment 3 Sean Stangl [:sstangl] 2011-10-13 11:50:30 PDT
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.
Comment 4 Nicolas B. Pierron [:nbp] 2011-10-18 10:35:37 PDT
Created attachment 567791 [details] [diff] [review]
Add setABIArg function with MoveOperand argument.

Update (old patch, r=sstangl)
Comment 5 David Anderson [:dvander] 2011-10-20 13:39:02 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/ae2ac74f31ae

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