877559 - IonMonkey: callVM passes floating point arguments incorrectly on x64

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

The x64 call ABI we use %rdi, %rsi, %rdx, etc and %xmm0-7 for the first couple of int/float arguments. The ordinal for the argument is dependent on type. That is, if I have a function:

  void f(int x, double d, int y);

I'll have the following argument mapping:

  x => %rdi
  d => %xmm0
  y => %rsi

The callVM mechanism doesn't distinguish int/float, and gives the following assignment, incorrectly calling any VMFunction with a double argument:

  x => %rdi
  d => %rsi
  y => %rdx

Comment 1

[Shu-yu Guo [:shu]]

Attachment: v0

Some background for why I need this: I'm trying to make IntToString and DoubleToString operations work in parallel. Calling the VMFunction for js_NumberToString, which takes a double, was breaking horribly.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 755786 [details] [diff] [review]
v0

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

Please provide the ARM equivalent of it before landing, and check that this is working on Try.

::: js/src/ion/VMFunctions.h
@@ +72,5 @@
>      uint32_t argumentProperties;
>  
> +    // Which arguments should be passed in float register on platforms that
> +    // have them.
> +    uint32_t argumentPassedInFloatRegs;

You might pack it in the argumentProperties by using 3 bits instead of 2.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +521,1 @@
>      // Reserve space for the outparameter.

nit: do not add extra new lines.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 755786 [details] [diff] [review]
v0

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

I think you need to implement the ARM variant of it otherwise the tree might turn red on ARM if we ever check JIT tests there.
Unless you want to do that a separated bug, in which case you can r=me, but you will need to do it before adding the VMFunction which has a double argument.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +557,5 @@
>                case VMFunction::WordByValue:
> +                if (f.argPassedInFloatReg(explicitArg))
> +                    masm.passABIArg(MoveOperand(argsBase, argDisp, MoveOperand::FLOAT));
> +                else
> +                    masm.passABIArg(MoveOperand(argsBase, argDisp));

You should do a similar modification on DoubleByValue for ARM.  Hopefully it should work seamlessly, as passABIArg already handle the Double case.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: v1

Implemented for ARM, carrying nbp's r+.

I've asked for additional f? from mjrosenb though, since there's currently an assert saying double-sized value args in vm calls are not supported on ARM. I enabled them only for double-prec floating value args; not sure why double-word value arguments are not supported on ARM.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 757620 [details] [diff] [review]
v1

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

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +662,5 @@
> +            if (f.argPassedInFloatReg(explicitArg)) {
> +                masm.passABIArg(MoveOperand(argsBase, argDisp, MoveOperand::FLOAT));
> +                argDisp += sizeof(double);
> +            } else {
> +                JS_NOT_REACHED("VMCalls with non-floating double-word value arguments is not supported.");

It seems like this case should really never be hit. Any time we want to pass a jsval by value, it should actually be a handle for GC purposes.  Feel free to nuke this code.

also, this code was written back when the only ARM ABI supported was softfp, which would pass double values in pairs of gprs.

Comment 6

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/680d346756b7

try: https://tbpl.mozilla.org/?tree=Try&rev=4809ded80471

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/680d346756b7