IonMonkey: setABIArg should use MoveOperand instead of Register.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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

6 years ago
Assignee: general → npierron
(Assignee)

Comment 1

6 years ago
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.
Attachment #559451 - Flags: review?(sstangl)
(Assignee)

Comment 2

6 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
(Assignee)

Updated

6 years ago
Blocks: 692114
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

6 years ago
Created attachment 567791 [details] [diff] [review]
Add setABIArg function with MoveOperand argument.

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
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.