Last Comment Bug 709423 - IonMonkey: Extend callWithABI to work with float arguments
: IonMonkey: Extend callWithABI to work with float arguments
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Eddy Bruel [:ejpbruel]
:
Mentors:
Depends on:
Blocks: 705302 707927 716694 721245
  Show dependency treegraph
 
Reported: 2011-12-09 18:08 PST by Eddy Bruel [:ejpbruel]
Modified: 2012-03-01 17:22 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to be committed (25.52 KB, patch)
2012-01-26 10:14 PST, Eddy Bruel [:ejpbruel]
sstangl: review+
Details | Diff | Splinter Review
Patch to be committed (43.05 KB, patch)
2012-01-27 12:43 PST, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Patch to be committed (49.86 KB, patch)
2012-01-30 07:13 PST, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Patch to be committed (49.80 KB, patch)
2012-01-31 13:40 PST, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Patch to be committed (42.30 KB, patch)
2012-02-11 07:44 PST, Eddy Bruel [:ejpbruel]
dvander: review+
Details | Diff | Splinter Review
/home/mrosenberg/patches/armDoubleABICall-r0.patch (21.90 KB, patch)
2012-02-21 17:36 PST, Marty Rosenberg [:mjrosenb]
dvander: review+
Details | Diff | Splinter Review

Description Eddy Bruel [:ejpbruel] 2011-12-09 18:08:03 PST

    
Comment 1 Nicolas B. Pierron [:nbp] 2012-01-04 19:22:25 PST
This should not be necessary unless we change the call convention of callWithABI to use a fastcall instead of a cdecl.  In the mean time, Bug 715276 provides a solution where types are shipped to generateVMWrapper which allocate the right number of registers to pass doubles on x86 stack.

*** This bug has been marked as a duplicate of bug 715276 ***
Comment 2 David Anderson [:dvander] 2012-01-04 19:27:58 PST
I don't see how callWithABI currently works with double inputs.
Comment 3 Nicolas B. Pierron [:nbp] 2012-01-04 22:43:28 PST
(In reply to David Anderson [:dvander] from comment #2)
> I don't see how callWithABI currently works with double inputs.

I does not, but doubles are pushed on the stack as other arguments so we are just lying to it by telling it that we have an extra argument for each double as if we were using "void *, void *" instead of "double".
Comment 4 David Anderson [:dvander] 2012-01-05 12:19:28 PST
That won't work on x64, right?
Comment 5 Nicolas B. Pierron [:nbp] 2012-01-05 19:08:52 PST
It does, double are just passed through gpr & stack, but we don't count them twice, this is part of VMFunction machinery.
Comment 6 David Anderson [:dvander] 2012-01-05 19:21:48 PST
Discussed on IRC, seems like we'll want to support this in our lightweight calling mechanism as well (callVM is heavyweight). Math functions shouldn't be required to go through callVM.

It's always an option to make callWithABI look more like callVM, we could abstract VMFunction out to Signature or something, but that's not really needed for this bug.
Comment 7 Nicolas B. Pierron [:nbp] 2012-01-26 07:00:00 PST
I think ejpbruel already made some work on this bug, to implement some math functions working with doubles.  You should ask him …
Comment 8 Eddy Bruel [:ejpbruel] 2012-01-26 10:14:31 PST
Created attachment 591837 [details] [diff] [review]
Patch to be committed

This patch restricts setABIArg so that arguments can now only be passed in order. We already did this anyway, but making this restriction explicit is necessary in order to properly implement passing floats according to the 64-bit ABI on Linux.

(On Linux, whether a float argument is passed in a register or not, and in which register it should be passed when it does, is based on the number and types of arguments preceding it)
Comment 9 Sean Stangl [:sstangl] 2012-01-26 14:17:27 PST
Comment on attachment 591837 [details] [diff] [review]
Patch to be committed

Review of attachment 591837 [details] [diff] [review]:
-----------------------------------------------------------------

Comments are made in /x86/ but apply to all arches.

::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +87,5 @@
>      uint32 displacement = stackForCall + STACK_SLOT_SIZE;
>      stackAdjust_ = stackForCall + ComputeByteAlignment(displacement, StackAlignment);
>      dynamicAlignment_ = true;
>      reserveStack(stackAdjust_);
> +    nextArg_ = 0;

This needs to be in setupABICall().

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +53,5 @@
>  {
>      // Number of bytes the stack is adjusted inside a call to C. Calls to C may
>      // not be nested.
>      uint32 stackAdjust_;
> +    uint32 nextArg_;

Could we call this "argsPushed_"?

Also keep track of some "args_" as passed into setupABICall(). If we do this, then we can assert |args_ == argsPushed_| in callWithABI().

@@ +541,3 @@
>      void setupUnalignedABICall(uint32 args, const Register &scratch);
>  
>      // Arguments can be assigned to a C/C++ call in any order. They are moved

Comment needs update: arguments are added from left-to-right as they appear in the C prototype.

@@ +546,5 @@
>      // automatically adjusted. It is extremely important that esp-relative
>      // addresses are computed *after* setupABICall(). Furthermore, no
>      // operations should be emitted while setting arguments.
> +    void setABIArg(const MoveOperand &from);
> +    void setABIArg(const Register &reg);

Could we call this "setNextABIArg()"?
Comment 10 Eddy Bruel [:ejpbruel] 2012-01-27 12:43:00 PST
Created attachment 592219 [details] [diff] [review]
Patch to be committed

The following patch is still incomplete, but incorporates the changes proposed by sstangl, and also refactors the implementation of callWithABI so that the amount by which the stack has to be adjusted is calculated on the fly rather than up front. This is necessary because in general, integers and doubles do not have the same size and we can't predict up front how many arguments of each type will be passed.

The next step is to refactor both passABIArg and callWithABI so that they know about double arguments. In particular, passABIArg needs to pass arguments via the proper registers on ARM and x64, and callWithABI needs to move the result from the x87 stack to an xmm register on x86, so that IonMonkey can access it.

Could anybody give this patch a quick review and check for general retardedness before I continue with the rest?
Comment 11 Sean Stangl [:sstangl] 2012-01-27 15:34:28 PST
Comment on attachment 592219 [details] [diff] [review]
Patch to be committed

Review of attachment 592219 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK. Comments on the x86 files apply to the other arches as well.

::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ -46,5 @@
>  using namespace js;
>  using namespace js::ion;
>  
> -uint32
> -MacroAssemblerX86::setupABICall(uint32 args)

It may still be useful to keep around setupABICall() as a private function, to host the common behavior between setupAlignedABICall() and setupUnalignedABICall().

@@ +51,5 @@
>  {
>      JS_ASSERT(!inCall_);
>      inCall_ = true;
>  
> +    args_ = 0;

args_ = args;

In fact, args_ can be DebugOnly<uint32>. It exists only so that we can JS_ASSERT(args_ == passedArgs_) in callWithVM.

@@ +66,2 @@
>  
> +    args_ = 0;

args_ = args;

@@ +76,3 @@
>  {
> +    stackForCall_ += sizeof(void *);
> +    uint32 disp = GetArgStackDisp(passedArgs_++);

GetArgStackDisp() is just stackForCall_ before it's incremented.

Perhaps the act of getting the current depth and incrementing it by some size can be handled by a private helper function? ArgAlloc(size_t)?

@@ +102,5 @@
> +        andl(Imm32(~(StackAlignment - 1)), esp);
> +        push(scratch_);
> +
> +        displacement = stackForCall + STACK_SLOT_SIZE;
> +    } else

Style guide mandates { } braces for |else| block because |if| block has them.

@@ +103,5 @@
> +        push(scratch_);
> +
> +        displacement = stackForCall + STACK_SLOT_SIZE;
> +    } else
> +        displacement = stackForCall + framePushed_;

This won't compile -- missing underscores. Make sure to test on all the platforms, debug and opt builds!

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +565,3 @@
>  
>      Register temp = regs.getAny();
>      masm.setupUnalignedABICall(f.argc(), temp);

The code between setupUnalignedABICall() and the first passABIArg() must be moved to before the setupUnalignedABICall(). No non-passABIArg() instructions can occur between setup and call.
Comment 12 Eddy Bruel [:ejpbruel] 2012-01-30 07:13:18 PST
Created attachment 592705 [details] [diff] [review]
Patch to be committed

This patch incorporates most of the changes proposed by sstangl, and refactors passABIArg and callWithABI for x86 and x64. Like I explained earlier, passABIArg needs to pass arguments via the proper registers on ARM and x64, and callWithABI needs to move the result from the x87 stack to an xmm register on x86, so that IonMonkey can access it.

If it works, this should be enough to add support for doubles to callWithABI for x86 and x64. However, note that currently we can only use passABIArg to pass doubles if the argument is stored in a register, because MoveOperand is unable to distinguish if an addresses contains a double or int. We might want to extend it so that it does, otherwise this could lead to surprises.

Other than that, the only thing left to do, afaict, is refactor passABIArg and callWithABI for arm as well. This might take a bit longer though, since I'm not as familiar with that platform.

Although this patch is still completely untested, I'd appreciate it if somebody could take another look at it, and check for overall sanity.
Comment 13 Eddy Bruel [:ejpbruel] 2012-01-31 13:40:18 PST
Created attachment 593209 [details] [diff] [review]
Patch to be committed

Looks like I somehow messed up that last patch. This should fix that.
Comment 14 Sean Stangl [:sstangl] 2012-01-31 15:17:55 PST
Comment on attachment 593209 [details] [diff] [review]
Patch to be committed

Review of attachment 593209 [details] [diff] [review]:
-----------------------------------------------------------------

Looked at x86 version only, assuming they're roughly equivalent.

::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-    1   callWithABI(void *fun, Result result)
> + * vim: set ts=4 sw=4 et tw=79:     

This is... interesting! There are also some hidden extra spaces added.

@@ +51,5 @@
>  {
>      JS_ASSERT(!inCall_);
>      inCall_ = true;
>  
> +    args_ = args;

if args_ is DebugOnly, assignments to args_ also need to be only for debug mode. #ifdef DEBUG?

@@ +53,5 @@
>      inCall_ = true;
>  
> +    args_ = args;
> +    passedArgs_ = 0;
> +    stackForCall_ = 0;

If all the above were in a private function setupABICall(), then this function could just be:

{
     setupABICall(args);
     dynamicAlignment_ = false;
}

@@ +70,2 @@
>      dynamicAlignment_ = true;
> +    scratch_ = scratch;

And this function could just be:

{
    setupABICall(args);
    dynamicAlignment_ = true;
    scratch_ = scratch;
}

@@ +79,5 @@
> +    if (from.isDouble()) {
> +        stackForCall_ += sizeof(double);
> +    } else {
> +        stackForCall_ += sizeof(int);
> +    }

nit: braces not needed if every branch is a single statement.

@@ +152,5 @@
> +    if (result == Double) {
> +        reserveStack(sizeof(double));
> +        fstp(Operand(esp, 0));
> +        movsd(Operand(esp, 0), ReturnFloatReg);
> +        freeStack(sizeof(double));

reserveStack() and freeStack() shouldn't be called -- instead, use a negative offset from %esp. I'm unsure of the convention for returning floats on-stack in x86, so I'm just trusting this is accurate since it will become pretty apparent if it's not.

@@ +170,5 @@
>      movl(esp, eax);
>  
>      // Ask for an exception handler.
>      setupUnalignedABICall(1, ecx);
> +    setABIArg(eax);

pass?

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +76,5 @@
>      typedef MoveResolver::MoveOperand MoveOperand;
>      typedef MoveResolver::Move Move;
>  
> +    enum Result {
> +        Default,

"Default" doesn't mean anything -- would the options "Register" and "FloatRegister" make more sense?
Comment 15 Eddy Bruel [:ejpbruel] 2012-02-11 07:44:06 PST
Created attachment 596326 [details] [diff] [review]
Patch to be committed

This patch adds support for doubles to the callWithABI API on x86 and x64. One caveat: its currently not possible to pass in double arguments from a memory location, since MoveOperand cannot tell if this location contains a double or not. If I need this feature in order to add double support to JSOP_MOD, I will implement it as part of that bug.

I also haven't added support for ARM yet; I've been struggling to get a working dev environment for ARM, so I will do this in a separate bug once I get one.

I've tested the patch with jit-tests.py --ion-tbpl on both x86 and x64 and it doesn't break any tests that weren't already failing with the patch not applied, so it should be ready for push once dvander r+'s.
Comment 16 David Anderson [:dvander] 2012-02-13 15:17:04 PST
Comment on attachment 596326 [details] [diff] [review]
Patch to be committed

Review of attachment 596326 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work. Looks good, but we'll need ARM parity to land without breaking.

::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +149,5 @@
> +    if (result == DOUBLE) {
> +        reserveStack(sizeof(double));
> +        fstp(Operand(esp, 0));
> +        movsd(Operand(esp, 0), ReturnFloatReg);
> +        breakpoint();

Remember to take this out before landing.
Comment 17 Eddy Bruel [:ejpbruel] 2012-02-13 16:31:15 PST
I could implement the passABIArg API for ARM without adding float support yet. That should allow us to land this patch without breaking existing ARM code. Would that work for you?
Comment 18 Paul Wright 2012-02-20 11:08:03 PST
Anything needed here beyond comment 17?
Comment 19 ejpbruel 2012-02-20 13:44:27 PST
(In reply to Paul Wright from comment #18)
> Anything needed here beyond comment 17?

Yeah, we still need ARM support. I think mjrosenb was supposed to look into it, because I don't have a working Android stack yet (so I can't test anything). I'm actually in Mountain View this week, so I plan to meet with mjrosenb somewhere this week to get my Android stack up. Once I do, I can implement ARM support myself.

Other than that, this patch is ready to land.
Comment 20 Marty Rosenberg [:mjrosenb] 2012-02-21 17:36:34 PST
Created attachment 599423 [details] [diff] [review]
/home/mrosenberg/patches/armDoubleABICall-r0.patch

ARM parts of the patch,
Also, while perusing the x86 patch, it looks like part of the license block from
MacroAssembler-x64.cpp was removed.
Comment 21 David Anderson [:dvander] 2012-02-21 18:54:18 PST
Comment on attachment 599423 [details] [diff] [review]
/home/mrosenberg/patches/armDoubleABICall-r0.patch

Review of attachment 599423 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1977,4 @@
>      }
> +
> +    if (useResolver)
> +        enoughMemory_ = enoughMemory_ && moveResolver_.addMove(from, dest, kind);

this can be enoughMemory_  &=

@@ +2009,5 @@
>  {
>      JS_ASSERT(inCall_);
> +    uint32 stackAdjust = ((usedSlots_ - 4 > 0) ? usedSlots_ - 4 : 0) * STACK_SLOT_SIZE;
> +    if (!dynamicAlignment_)
> +        stackAdjust += 8-(framePushed_ & 7);

style nit: 8 - (framePushed_ & 7)

@@ +2024,5 @@
>          emitter.finish();
>      }
> +    for (int i = 0; i < 2; i++) {
> +        if (!floatArgsInGPR[i].isInvalid()) {
> +            ma_vxfer(floatArgsInGPR[i], Register::FromCode(i*2), Register::FromCode(i*2+1));

style nit: i * 2, i * 2 + 1
Comment 22 Jan de Mooij [:jandem] (PTO until July 31) 2012-02-24 10:08:22 PST
Eddy, can you land these patches, or do you need somebody to land them for you? Bug 705302 blocks our slowest SS test + the Kraken crypto-tests, so I'd really like to get this in.
Comment 23 Eddy Bruel [:ejpbruel] 2012-02-24 10:33:15 PST
(In reply to Jan de Mooij (:jandem) from comment #22)
> Eddy, can you land these patches, or do you need somebody to land them for
> you? Bug 705302 blocks our slowest SS test + the Kraken crypto-tests, so I'd
> really like to get this in.

I'm currently a bit preoccupied with the Jetpack workweek. Iĺl try to sneak in some free time to land this, but I cant promise anything. Feel free to land it for me if you really need it ASAP (make sure you remove the redundant stuff dvander mentioned in comment 16). Ill be available again tuesday next week. If you can wait that long, I will definitely be able to land the patches then.
Comment 24 Jan de Mooij [:jandem] (PTO until July 31) 2012-02-24 10:49:54 PST
(In reply to Eddy Bruel [:ejpbruel] from comment #23)
> 
> Ill be available again tuesday next week. If you can wait that long, I will definitely be able to land the patches then.

Next week is fine, there's enough work to keep us occupied until then ;) Thanks!
Comment 25 Paul Wright 2012-03-01 16:44:34 PST
Ping
Comment 27 Eddy Bruel [:ejpbruel] 2012-03-01 17:22:59 PST
Oops! I forgot to land this patch as I promised. Sorry jandem!

Thanks for landing it for me, dvander.

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