Closed Bug 709423 Opened 12 years ago Closed 12 years ago

IonMonkey: Extend callWithABI to work with float arguments

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(3 files, 3 obsolete files)

      No description provided.
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
I don't see how callWithABI currently works with double inputs.
(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".
That won't work on x64, right?
It does, double are just passed through gpr & stack, but we don't count them twice, this is part of VMFunction machinery.
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 716694
Blocks: 721245
Assignee: general → sstangl
I think ejpbruel already made some work on this bug, to implement some math functions working with doubles.  You should ask him …
Attached patch Patch to be committed (obsolete) — Splinter Review
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)
Attachment #591837 - Flags: review?(sstangl)
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()"?
Attachment #591837 - Flags: review?(sstangl) → review+
Attached patch Patch to be committed (obsolete) — Splinter Review
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?
Attachment #591837 - Attachment is obsolete: true
Attachment #592219 - Flags: review?(dvander)
Attachment #592219 - Attachment is patch: true
Attachment #592219 - Flags: review?(dvander) → review?(sstangl)
Assignee: sstangl → ejpbruel
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.
Attachment #592219 - Flags: review?(sstangl)
Attached patch Patch to be committed (obsolete) — Splinter Review
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.
Attachment #592705 - Flags: review?(sstangl)
Looks like I somehow messed up that last patch. This should fix that.
Attachment #592219 - Attachment is obsolete: true
Attachment #592705 - Attachment is obsolete: true
Attachment #592705 - Flags: review?(sstangl)
Attachment #593209 - Flags: review?(sstangl)
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?
Attachment #593209 - Flags: review?(sstangl)
Blocks: 705302
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.
Attachment #596326 - Flags: review?(dvander)
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.
Attachment #596326 - Flags: review?(dvander) → review+
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?
Anything needed here beyond comment 17?
(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.
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.
Attachment #599423 - Flags: review?(dvander)
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
Attachment #599423 - Flags: review?(dvander) → review+
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.
(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.
(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!
Ping
Oops! I forgot to land this patch as I promised. Sorry jandem!

Thanks for landing it for me, dvander.
You need to log in before you can comment on or make changes to this bug.