904918 - OdinMonkey: add support for Float32

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

This bug will add Float32 support and tests for OdinMonkey and in asm.js type system.

So far, the type checking rules are almost the same as the ones for Doubles, except the following:
- coercion from floatish to float is done by either a call to Math.toFloat32, which has to be imported, or a store to a Float32 array.
- addition and subtractions can't have more than 2 operands (if a, b, c are Float32, a + b + c is not legal, for instance). The reason for that is there can be a loss of precision. Such additions and subtractions should coerce intermediary results between all operations: |toFloat32(a + b) + c| is legal and returns floatish. Also, |a + toFloat32(b + c)| is legal but can not have the same result.
- a function cannot return a Float32, so it has to get converted to a Double or to an Int before it is returned.

There might be other differences I am not thinking at right now, so anybody should feel free to add them as comments.

Comment 1

[Alon Zakai (:azakai)]

Will we be able to get performance numbers without asm.js type system changes, or do we need them to get a realistic picture?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Alon Zakai (:azakai) from comment #1)
> Will we be able to get performance numbers without asm.js type system
> changes, or do we need them to get a realistic picture?

In a previous patch (never posted as highly experimental), I changed Odinmonkey so that it emits Float32 every time we would emit a Double, and observed the same speed-ups as described in bug 878170.

If you're thinking about performance numbers regarding non-asm.js scripts (float32 in Ion only), on box2d for instance, I haven't checked yet but plan to do it soon.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: A - Type System

This patch implements the type checks for Float32 in Odin, assuming that all MIR nodes exist and can be specialized for Float32 without segfaulting the shell. The patch in its current state doesn't compile by itself.

I'll add patches implementing MIR nodes progressively with their x86/x64 implementation (have to split them and specialize them so that they can still work in Ion, first).

Some comments regarding Float32 special behaviors:
- For Math builtin functions, if the RetType is Float and the function has a float32 C++ specialization (which is the case for all Math builtin with an arity of one), use the float32 function instead of the double equivalent.
- Float32 store can take a double (that will get converted) as an input, to allow |f32[0] = 0|.

Luke, could you please give a look and tell me if the design looks good and implementation follows the spec we talked about?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: B - JIT tests

And some tests for the type system. With the MIR patches that I haven't added yet, they all pass on my machine \o/ (and also on try for x86 ad x64: https://tbpl.mozilla.org/?tree=Try&rev=2a3b59649eb3 )

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 793227 [details] [diff] [review]
A - Type System

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

Great job!  I'm happy to see that this ended up fitting in pretty cleanly.

::: js/src/jit/AsmJS.cpp
@@ +1389,5 @@
>      ExitMap::Range allExits() const {
>          return exits_.all();
>      }
>  
> +    bool isToFloat32Call(ParseNode *pn, bool reportError = false, const char *msg = NULL, PropertyName *name = NULL)

It's great that you factored this out!  I like to keep all the type-checking logic in non-member Check* functions.  Could you perhaps name this CheckFloat32Coercion and put it right before the first use (looks like CheckTypeAnnotation)?

@@ +1395,5 @@
> +        JS_ASSERT(pn->isKind(PNK_CALL));
> +
> +        // XXX(benjamin) we want to print the property name in the error context message,
> +        // if there is a property name, so we need a buffer to save the final message. I am
> +        // not entirely satisfied with this approach and can't see any easy alternative.

Good news, there is!  Check out 'failName' and uses :)

@@ +1410,5 @@
> +                strcpy(errorContext, msg);
> +            }
> +        }
> +
> +        ParseNode *callee = CallCallee(pn);

Can you rename 'pn' to 'callNode'?

@@ +1413,5 @@
> +
> +        ParseNode *callee = CallCallee(pn);
> +        if (!callee->isKind(PNK_NAME)) {
> +            if (reportError)
> +                return failf(callee, "%s, unexpected callee", errorContext);

Instead of having CheckFloat32Coercion take a 'reportError', let's just have it unconditionally report; it'll make this function a good bit cleaner.  The only caller passing 'false' is CheckCallExpr() but really I think if we changed this message and the one below to "expecting coercive call to roundFloat32" then I think we'll be fine.

@@ +1417,5 @@
> +                return failf(callee, "%s, unexpected callee", errorContext);
> +            return false;
> +        }
> +
> +        PropertyName *calleeName = callee->name();

Can you put this on its own line since its conceptually separate from the global-probing code that follows?

@@ +1430,5 @@
> +
> +        unsigned numArgs = CallArgListLength(pn);
> +        if (numArgs != 1) {
> +            if (reportError)
> +                return failf(callee, "%s, wrong arguments to Math.roundFloat32", errorContext);

Probably don't need to pass the name here, instead (borrowing from CheckMathBuiltinCall):
  return f.failf(callNode, "roundFloat32 passed %u arguments, expected %u", numArgs, 1);
?

@@ +1885,5 @@
>          }
>          unsigned firstLocalSlot = argTypes.length();
>          for (unsigned i = 0; i < varInitializers_.length(); i++) {
> +            MConstant *ins = MConstant::New(varInitializers_[i].value);
> +            ins->setResultType(varInitializers_[i].type.toMIRType());

The pattern I used for MIR nodes that get created with an initial type is to have a NewAsmJS static function that does the setResultType call.

@@ +2046,5 @@
>              return;
> +
> +        if (vt == ArrayBufferView::TYPE_FLOAT32 && v->type() != MIRType_Float32) {
> +            // can happen if we try to store a double directly
> +            // XXX(benjamin) could also be done by adding a type policy to AsmJSStoreHeap, which would avoid this manual insertion here

Is this whole branch still necessary with the recent changes to CheckStoreArray?

@@ +2772,2 @@
>      NumLit literal = ExtractNumericLiteral(initNode);
> +

Can you keep this the same?  I kinda like to have declaration right before the switch that initializes them.

@@ +2817,5 @@
> +            return false;
> +
> +        *coercion = AsmJS_ToFloat32;
> +        if (coercedExpr)
> +            *coercedExpr = CallArgList(coercionNode);

So what I think you want to do is make CheckFloat32Coercion take a "ParseNode **coercedExpr" argument (like CheckTypeAnnotation) so that we keep the "how a float32 coercion is represented" logic in one place.

@@ +2827,5 @@
>      return m.fail(coercionNode, "in coercion expression, the expression must be of the form +x or x|0");
>  }
>  
>  static bool
> +CheckTypedGlobalVariableInitImport(ModuleCompiler &m, PropertyName *varName, AsmJSCoercion &coercion, ParseNode *coercedExpr)

How about the shorter CheckImportExpr?

@@ +2868,5 @@
> +            return m.fail(arg, "numeric literal argument to roundFloat32 must be a double");
> +        return m.addGlobalVarInitConstant(varName, VarType::Float, literal.value());
> +    } else if (arg->isKind(PNK_DOT)) {
> +        AsmJSCoercion coercion = AsmJSCoercion::AsmJS_ToFloat32;
> +        return CheckTypedGlobalVariableInitImport(m, varName, coercion, arg);

I think you can make this a single line and still fit in 100 chars.  Second, the SM style is to not have 'else' when the 'if' branch ends with return.

@@ +3010,5 @@
>  static bool
>  ArgFail(FunctionCompiler &f, PropertyName *argName, ParseNode *stmt)
>  {
>      return f.failName(stmt, "expecting argument type declaration for '%s' of the "
> +                      "form 'arg = arg|0' or 'arg = +arg' or 'arg = Math.roundFloat32(arg)'", argName);

Can you s/Math.roundFloat32/roundFloat32/ since they can't actually write the 'Math.'.

@@ +3135,4 @@
>          return f.failName(initNode, "initializer for '%s' needs to be a numeric literal", name);
> +    }
> +
> +    ParseNode *valueNode = (isFloat32Call) ? CallArgList(initNode) : initNode;

Instead of two if/elses separated by the ExtractNumericLiteral call, I think we can duplicate the ExtractNumericLiteral call and have a single simpler if/else (with no 'isFloat32Call' variable).

@@ +3687,5 @@
>  {
>      PropertyName *calleeName = CallCallee(callNode)->name();
>  
> +    if (retType == RetType::Float)
> +        return f.fail(callNode, "FFI calls can't return Float32");

"can't return float"

@@ +3707,5 @@
>  
> +static bool CheckCall(FunctionCompiler &f, ParseNode *call, RetType retType, MDefinition **def, Type *type);
> +
> +static bool
> +CheckToFloat32(FunctionCompiler &f, ParseNode *expr, MDefinition **def, Type *type)

Could this be CheckRoundFloat32?

@@ +3710,5 @@
> +static bool
> +CheckToFloat32(FunctionCompiler &f, ParseNode *expr, MDefinition **def, Type *type)
> +{
> +    JS_ASSERT(expr->isKind(PNK_CALL));
> +    ParseNode *input = CallArgList(expr);

Could you s/input/arg/?

@@ +3716,5 @@
> +    if (input->isKind(PNK_CALL))
> +        return CheckCall(f, input, RetType::Float, def, type);
> +
> +    if (IsNumericLiteral(input) && ExtractNumericLiteral(input).which() != NumLit::Double)
> +        return f.fail(input, "numeric literal argument in call to Math.roundFloat32 should be a double");

I don't think we should single out literal expressions in this way: 'toFloat32(1)' doesn't seem any worse than 'toFloat32(f()|0)'.

@@ +3737,5 @@
> +    return true;
> +}
> +
> +static bool
> +CheckCoercedCallToFloat32(FunctionCompiler &f, ParseNode *callNode, RetType retType, MDefinition **def, Type *type)

This is quite an interesting corner case!  I assume there are tests :)  Can you name it CheckCoercedRoundFloat32?

@@ +3757,5 @@
> +    } else if (retType == RetType::Float) {
> +        *def = operand;
> +        *type = Type::Float;
> +    } else {
> +        return f.fail(callNode, "only legal coercions from Float32 are double or signed");

Could you use a switch(retType.which()) here (so we get warnings if new enumerators are added later)?

Also, I think 'void' should type check.  Don't you get here if you write: "roundFloat32(x);" or "(roundFloat32(x), 1);"?

@@ +3800,5 @@
>                       RetType retType, MDefinition **def, Type *type)
>  {
>      unsigned arity = 0;
>      void *callee = NULL;
> +    bool useFloat = retType == RetType::Float;

I think we could simplify this function a bit by avoiding the 'useFloat' bool and changing how we check the return type.  How about:

double (*doubleFunc)(double) = NULL;
float (*floatFunc)(float) = NULL;
switch (mathBuiltin) {
  case AsmJSMathBuiltin_sin:  arity = 1; doubleFunc = sin; floatFunc = sinf; break;
  ...
  case AsmJSMathBuiltin_pow:  arity = 2; doubleFunc = ecmaPow; break;
}

if (retType == RetType::Float && !floatFunc)
   return f.fail("math builtin cannot be used as a float");
if (retType != RetType::Double && retType != RetType::Float)
   return f.fail("return type of math function is double or float, used as %s", retType.toType().toChars());

Then after that, you can use retType for everything w/o (useFloat?:) expressions.

@@ +4144,5 @@
> +    if (!lhsType.isFloatish() || !rhsType.isFloatish())
> +        return f.fail(star, "multiply operands must be both int, both doublish or both floatish");
> +
> +    *def = f.mul(lhsDef, rhsDef, MIRType_Float32, MMul::Normal);
> +    *type = Type::Floatish;

To keep the double and float cases symmetric, can you write:

if (lhsType.isFloatish() || rhsType.isFloatish())
   ...

return f.fail(...);

@@ +4166,5 @@
>      if (lhs->isKind(PNK_ADD) || lhs->isKind(PNK_SUB)) {
>          if (!CheckAddOrSub(f, lhs, &lhsDef, &lhsType, &lhsNumAddOrSub))
>              return false;
> +        if (lhsType == Type::Floatish)
> +            return f.fail(lhs, "float32 additions and subtractions can't be chained without coercions");

IIUC, this test isn't necessary since we'll end up in the failure case below.  (Same for the rhsType test below.)

@@ +4256,5 @@
> +            *def = f.binary<MDiv>(lhsDef, rhsDef, MIRType_Float32);
> +        else
> +            return f.fail(expr, "modulo cannot receive Float32 arguments");
> +
> +        *type = Type::Floatish;

For symmetry with the preceeding Doublish case, can you remove the empty line above?

::: js/src/jit/AsmJSModule.h
@@ +25,5 @@
>  enum AsmJSCoercion
>  {
>      AsmJS_ToInt32,
> +    AsmJS_ToNumber,
> +    AsmJS_ToFloat32

Even though it breaks symmetry, could you name it AsmJS_RoundFloat32 instead (since we are naming the coercion that was applied)?

::: js/src/jsapi.h
@@ +1569,5 @@
>  }
>  
> +/* ES5 9.3 ToNumber. */
> +JS_ALWAYS_INLINE bool
> +ToFloatNumber(JSContext *cx, const Handle<Value> &v, float *out)

I don't think we should expose this function in jsapi.h.  Instead, can you move this to jsmath.h, name it RoundFloat32, ditch the current function body (which is only appropriate for jsapi.h), factor out the body with math_roundFloat32, and give this a comment that says "Implements Math.roundFloat32"?

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 793266 [details] [diff] [review]
B - JIT tests

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

Very nice tests!

Have you tested with Emscripten FLOAT32 output yet?  I'm excited to see what this does on the awfy benchmarks.

::: js/src/jit-test/tests/asm.js/testFloat32.js
@@ +2,5 @@
> +const TO_FLOAT32 = "var toF = glob.Math.roundFloat32;";
> +
> +function equalFloat(a,b)
> +{
> +    return (isNaN(a) && isNaN(b)) || a == b || Math.abs(a-b) < 0.0001;

It really seems like, if our commutative property holds, we shouldn't need this.  The only exception is the standard math library where double/float can product different values but this is generally acceptable and allowed by the standard.  If that is the case, could you move this assertEq patching to the end of the test, right before the math tests and add a comment explaining?

@@ +39,5 @@
> +assertAsmTypeFail('glob', USE_ASM + TO_FLOAT32 + "function f() { var i = toF(); } return f");
> +assertAsmTypeFail('glob', USE_ASM + TO_FLOAT32 + "function f() { var i = toF(x, x); } return f");
> +assertAsmTypeFail('glob', USE_ASM + TO_FLOAT32 + "function f() { var i = toF('hi'); } return f");
> +assertAsmTypeFail('glob', USE_ASM + TO_FLOAT32 + "function f() { var i = toF(5); } return f");
> +assertEq(asmLink(asmCompile('glob', USE_ASM + TO_FLOAT32 + "function f() { var i = toF(5.); } return f"), this)(), undefined);

It occurs to me reading this that 'var i = toF(5)' should be allowed since I can't really think of a reason not to.  We require the . in "var x = 0.0" to distinguish int from double, but that's no issue here.  Also, the non-var statement 'toF(5);' type checks fine because toF coerces ints/doubles.

@@ +42,5 @@
> +assertAsmTypeFail('glob', USE_ASM + TO_FLOAT32 + "function f() { var i = toF(5); } return f");
> +assertEq(asmLink(asmCompile('glob', USE_ASM + TO_FLOAT32 + "function f() { var i = toF(5.); } return f"), this)(), undefined);
> +
> +// Return values
> +assertAsmTypeFail('glob', USE_ASM + TO_FLOAT32 + "function f() { return toF(4); } return f");

This line should succeed after the change suggested in the previous patch.

::: js/src/jit-test/tests/asm.js/testHeapAccess.js
@@ +1,4 @@
>  load(libdir + "asm.js");
>  
>  assertAsmTypeFail('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { i32[0>>2] = 4.0; return i32[0>>2]|0; } return f');
> +assertAsmTypeFail('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { f64[0>>2] = 4; return +f64[0>>2]; } return f');

Shouldn't this still fail to type check?  4 isn't a floatish or doublish.

@@ +146,5 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { f64[(8&0xffff)>>3] = 1.3 } return f'), this, null, BUF_64KB)();
>  assertEq(new Float64Array(BUF_64KB)[1], 1.3);
>  
>  new Float32Array(BUF_64KB)[1] = 1.0;
> +assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'var toFloat32 = glob.Math.roundFloat32; function f() { return toFloat32(f32[(4&0xffff)>>2]) } return f'), this, null, BUF_64KB)(), 1.0);

Shouldn't this original still type check?  (But do add the roundFloat32 test.)

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1 - AsmJS nodes MIR / LIR / Codegen

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - Type System

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 3 - JIT tests

Not sure I need to ask for review for tests, but these allow to have a quick overview of the new spec.

Comment 10

[Sean Stangl [:sstangl]]

Comment on attachment 801939 [details] [diff] [review]
Part 1 - AsmJS nodes MIR / LIR / Codegen

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

lgtm

::: js/src/jit/shared/Lowering-x86-shared.cpp
@@ +201,5 @@
>  LIRGeneratorX86Shared::visitAsmJSNeg(MAsmJSNeg *ins)
>  {
>      if (ins->type() == MIRType_Int32)
>          return defineReuseInput(new LNegI(useRegisterAtStart(ins->input())), ins, 0);
> +    else if(ins->type() == MIRType_Float32)

no else after return

::: js/src/jit/x86/CodeGenerator-x86.cpp
@@ +465,5 @@
>      Register ptr = ToRegister(ins->ptr());
>      const LDefinition *out = ins->output();
>  
>      OutOfLineLoadTypedArrayOutOfBounds *ool = NULL;
> +    bool isFloat32Load = vt == ArrayBufferView::TYPE_FLOAT32;

prefer parentheses on rhs of "="

@@ +529,5 @@
>  
>  bool
>  CodeGeneratorX86::visitOutOfLineLoadTypedArrayOutOfBounds(OutOfLineLoadTypedArrayOutOfBounds *ool)
>  {
> +    static float js_NaN_float = js_NaN;

I would add this to jsnum.cpp, js_NaN_float32.

@@ +693,5 @@
>          return;
>  
> +    if (mir->type() == MIRType_Float32) {
> +        masm.reserveStack(sizeof(float));
> +        masm.fstp32(Operand(esp, 0));

It occurs to me that we don't need to actually call reserveStack() and freeStack(), since we can just use the address at |Operand(esp, -(sizeof(float))|, which is like an implicit reservation. Likewise for the double case.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1 - AsmJS nodes MIR / LIR / Codegen + fixed nits

Nice trick for the postAsmJSCall function! Thanks for the review.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1 - AsmJS nodes MIR / LIR / Codegen - rebased

Carrying forward r+ from sstangl. Rebased as of today.

Comment 13

[Douglas Crosher [:dougc]]

Attachment: x86 and x64 backend support

Just rebasing to give something the use for writing and testing the ARM support.  Have not considered feedback etc.  It compiles, passes the included tests, and the usual asm.js game demos seem to run ok.

Comment 14

[Douglas Crosher [:dougc]]

Attachment: Type system

More of the rebasing.

Comment 15

[Douglas Crosher [:dougc]]

Attachment: Tests

Rebased tests.

Comment 16

[Luke Wagner [:luke]]

Thanks a lot Douglas.  What we're waiting on here is to get Emscripten generating float32 code and Emscripten is waiting to get feedback from other browsers.  The next step is to write a blog post explaining the float32 optimization and giving demos.  We're working on that :)  For now, it'd be good to get the rest of the general float32 optimizations in the other bugs landed.

Comment 17

[Douglas Crosher [:dougc]]

Attachment: x86 and x64 backend support

Rebased.

Comment 18

[Douglas Crosher [:dougc]]

Attachment: type system

Rebased.

Comment 19

[Douglas Crosher [:dougc]]

Attachment: tests

Rebased.

Comment 20

[Douglas Crosher [:dougc]]

Attachment: type system

Rebased

Comment 21

[Luke Wagner [:luke]]

Attachment: combined-odin-float32.patch

Combined patch for testing (applies to trunk at f78f52c8c9ff).

Comment 22

[Luke Wagner [:luke]]

Attachment: combined odin-float32.patch v.2

This version allows floats to be assigned to float64 arrays without explicit coercion.

Comment 23

[Luke Wagner [:luke]]

Attachment: combined-odin-float32.patch v.2

Oops, wrong patch.

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1 - AsmJS nodes MIR / LIR / Codegen - rebased

Rebased! Carrying forward r+ from sstangl

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: odin-asmjsnodes.patch

Fixed one rebasing bug and one pointer kind bug. Carrying forward r+ from sstangl

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - Type System - WIP

Rebased, plus:
- renamed *RoundFloat32* into *FRound*.
- added the coercion rule from comment 22.
- AsmJSImm now contains a few more float math functions. As pow and atan2 don't have float equivalent (for now), I introduced an "undefined" AsmJSImm that can be tested for knowing whether the function has a float equivalent or not. There might be room for improvement here.

Comment 27

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 3 - JIT tests - WIP

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - Type System - WIP

Rebased and updated:
- error message for calls.

Comment 29

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 3 - JIT tests

Updated:
- special cases for f32 (resp f64) stores when the input is a f64 (resp f32).

Comment 30

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - Type System

Rebased.

- Should we add support for module-constant initializers by the way? (for instance, function m(stdlib, ffi, heap){"use asm"; var x = 3.14; function f() {var y = x; return +y} return f})
- This still needs ARM support before landing.

Comment 31

[Luke Wagner [:luke]]

Comment on attachment 8337878 [details] [diff] [review]
Part 3 - JIT tests

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

Magnificent tests!

::: js/src/jit-test/tests/asm.js/testFloat32.js
@@ +55,5 @@
> +assertEq(asmLink(asmCompile('glob', 'ffi', 'heap', USE_ASM + TO_FLOAT32 + HEAP32 + "function f() { var i = toF(5.); f32[0] = toF(6.); i = toF(f32[0]); return toF(i); } return f"), this, null, heap)(), 6);
> +
> +// Special array assignments (the other ones are tested in testHeapAccess)
> +assertEq(asmLink(asmCompile('glob', 'ffi', 'heap', USE_ASM + TO_FLOAT32 + HEAP32 + "function f() { var i = 5.; f32[0] = i; return toF(f32[0]); } return f"), this, null, heap)(), 5);
> +assertEq(asmLink(asmCompile('glob', 'ffi', 'heap', USE_ASM + TO_FLOAT32 + HEAP32 + "var f64 = new glob.Float64Array(heap); function f() { var i = toF(5.); f64[0] = i; return +f64[0]; } return f"), this, null, heap)(), 5);

Maybe I missed it, but could you add a test for toF(f64[0]) (which should be valid)?

Comment 32

[Luke Wagner [:luke]]

Comment on attachment 8337878 [details] [diff] [review]
Part 3 - JIT tests

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

Oh, also I don't recall a test for the ternary operator so could you add one if there isn't already?

Comment 33

[Luke Wagner [:luke]]

Comment on attachment 8337878 [details] [diff] [review]
Part 3 - JIT tests

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

::: js/src/jit-test/tests/asm.js/testFloat32.js
@@ +180,5 @@
> +assertEq(asmLink(asmCompile('glob', USE_ASM + TO_FLOAT32 + "var g = 4; function s(x) { x = toF(x); g = (g + ~~x)|0; return toF(g|0);} function f(x) { x = toF(x); return (toF(s(x)), g)|0} return f"), this)(3), 7);
> +
> +// --> coerced calls
> +assertEq(asmLink(asmCompile('glob', USE_ASM + TO_FLOAT32 + "var g = 4; function s(x) { x = toF(x); g = (g + ~~x)|0; return +(g|0);} function f(x) { x = toF(x); return toF(+s(x))} return f"), this)(3), 7);
> +assertEq(asmLink(asmCompile('glob', USE_ASM + TO_FLOAT32 + "var g = 4; function s(x) { x = toF(x); g = (g + ~~x)|0; return g|0;} function f(x) { x = toF(x); return toF(s(x)|0)} return f"), this)(3), 7);

Maybe this is somewher else, but could you also have a test for the toF(toF(s(x))) case?

Comment 34

[Luke Wagner [:luke]]

Comment on attachment 8341998 [details] [diff] [review]
Part 2 - Type System

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

Very nice!  A few non-nits below, but I think this is almost ready to land.

::: js/src/jit/AsmJS.cpp
@@ +422,5 @@
> +        return isFloat() || which_ == MaybeFloat;
> +    }
> +
> +    bool isFloatish() const {
> +        return isMaybeFloat() || which_ == Floatish;

?

@@ +2047,5 @@
>          curBlock_->add(constant);
>          return constant;
>      }
>  
> +    MDefinition *constant(const Value &v, MIRType type)

How about
  MDefinition *constantFloat(float f)
?

@@ +2921,5 @@
> +CheckFloat32Coercion(ModuleCompiler &m, ParseNode *callNode, ParseNode **coercedExpr, PropertyName *name = nullptr)
> +{
> +    static const char* callError = "all function calls must either be ignored (via "
> +                                   "f(); or comma-expression), coerced to signed "
> +                                   "(via f()|0), coerced to float (via Math.fround(f()))"

I'd take out the "Math." from this string.  

I think this error message is only appropriate when the caller is Check(Uncoerced)CallExpr; the other contexts are variable initializers etc.  Perhaps you could have the caller pass in the error string as an argument?

Lastly, can you remove the 'name' parameter?

@@ +2977,5 @@
> +      case PNK_CALL: {
> +        if (!CheckFloat32Coercion(m, coercionNode, coercedExpr))
> +            return false;
> +
> +        *coercion = AsmJS_FRound;

For symmetry with the other cases, can you put the assignment to *coercion before the CheckFloat32Coercion call and then 'return CheckFloat32Coercion'?

@@ +2988,5 @@
>  }
>  
>  static bool
> +CheckImportExpr(ModuleCompiler &m, PropertyName *varName, const AsmJSCoercion &coercion,
> +                ParseNode *coercedExpr, bool isConst)

Could this be named CheckGlobalVariableImportExpr?  Also, you can just take 'coercion' by value, not const&.

@@ +3017,5 @@
> +    return CheckImportExpr(m, varName, coercion, coercedExpr, isConst);
> +}
> +
> +static bool
> +CheckDoubleNumericLiteral(ModuleCompiler &m, ParseNode *pn, NumLit *numLit, PropertyName *name = NULL)

Instead of doing a Check* style function, could you make this more like ExtractNumericLiteral and have a pair of functions:
  IsFRoundableLiteral
  ExtractFRoundableLiteral
?  (Saying "FRoundable" instead of "Double" seems preferable since a "Double Numeric Literal" is a NumLit::Double, which isn't the same thing.)  This would also allow callers to do their own error handling and avoid the optional 'name' parameter.  If this makes sense, could you put these functions right below IsNumericLiteral/ExtractNumericLiteral?

@@ +3028,5 @@
> +            return m.fail(pn, "initializer is out of range");
> +    }
> +
> +    if (literal.which() != NumLit::Double)
> +        literal = NumLit(NumLit::Double, DoubleValue(literal.toInt32()));

Could you do the case analysis with a switch statement (and no default: label)?

@@ +3052,5 @@
> +
> +    if (arg->isKind(PNK_DOT))
> +        return CheckImportExpr(m, varName, AsmJSCoercion::AsmJS_FRound, arg, isConst);
> +
> +    return m.fail(arg, "unsupported argument to fround");

I think you can just
  return CheckImportExpr(...)
since it already checks and reports if !arg->isKind(PNK_DOT).

@@ +3312,5 @@
> +        if (!IsNumericLiteral(coercedVar))
> +            return f.failName(coercedVar, "call to fround in initializer for '%s' must have a numerical argument", name);
> +
> +        NumLit literal;
> +        if (!CheckDoubleNumericLiteral(f.m(), coercedVar, &literal, name))

I think the above 4 lines would be collapsed with the IsFroundableLiteral change suggested above.

@@ +3316,5 @@
> +        if (!CheckDoubleNumericLiteral(f.m(), coercedVar, &literal, name))
> +            return false;
> +
> +        return f.addVariable(var, name, VarType::Float, literal.value());
> +    } else if (!IsNumericLiteral(initNode)) {

No 'else' after 'return'

@@ +3325,5 @@
>      VarType type;
>      switch (literal.which()) {
> +        case NumLit::Fixnum:
> +        case NumLit::NegativeInt:
> +        case NumLit::BigUnsigned:

Can you undo this indentation change?

@@ +3988,2 @@
>  static bool
> +CheckMathFRound(FunctionCompiler &f, ParseNode *arg, MDefinition **def, Type *type)

Perhaps we could name this CheckFRoundArg?  Second, I think both callers of this function call CheckFloat32Coercion right before, passing in coercedExpr as 'arg'.  Can we factor this out and put the CheckFloat32Coercion call at the top of CheckFRoundArg?

@@ +4019,5 @@
> +    return true;
> +}
> +
> +static bool
> +CheckMathFRound(FunctionCompiler &f, ParseNode *callNode, RetType retType, MDefinition **def, Type *type)

Perhaps we could name this just CheckFRound()?

@@ +4048,5 @@
> +        // definition and return types should be ignored by the caller
> +        return true;
> +    }
> +
> +    return f.fail(callNode, "return value of fround is ignored");

Can this be MOZ_ASSUME_UNREACHABLE?

@@ +4078,5 @@
> +      case AsmJSMathBuiltin_imul:    return CheckMathIMul(f, callNode, retType, def, type);
> +      case AsmJSMathBuiltin_abs:     return CheckMathAbs(f, callNode, retType, def, type);
> +      case AsmJSMathBuiltin_sqrt:    return CheckMathSqrt(f, callNode, retType, def, type);
> +      case AsmJSMathBuiltin_fround:  return CheckMathFRound(f, callNode, retType, def, type);
> +      case AsmJSMathBuiltin_sin:   arity = 1; doubleCallee = AsmJSImm_SinD; floatCallee = AsmJSImm_SinF;        break;

Can you put an extra space before 'arity' and take out a space before the 'return' to align these columns?

@@ +4507,5 @@
>      }
>  
> +    if (lhsType.isMaybeFloat() && rhsType.isMaybeFloat()) {
> +        if (expr->isKind(PNK_DIV))
> +            *def = f.div(lhsDef, rhsDef, MIRType_Float32, /* unsignd = */ false);

s/unsignd/unsigned/

@@ +4509,5 @@
> +    if (lhsType.isMaybeFloat() && rhsType.isMaybeFloat()) {
> +        if (expr->isKind(PNK_DIV))
> +            *def = f.div(lhsDef, rhsDef, MIRType_Float32, /* unsignd = */ false);
> +        else
> +            return f.fail(expr, "modulo cannot receive Float32 arguments");

s/Float32/float/

@@ +4644,5 @@
>      return true;
>  }
>  
>  static bool
> +CheckCallExpr(FunctionCompiler &f, ParseNode *expr, MDefinition **def, Type *type)

How about naming this CheckUncoercedCall?

@@ +4676,5 @@
>        case PNK_COMMA:       return CheckComma(f, expr, def, type);
>        case PNK_CONDITIONAL: return CheckConditional(f, expr, def, type);
>        case PNK_STAR:        return CheckMultiply(f, expr, def, type);
>  
> +      case PNK_CALL:        return CheckCallExpr(f, expr, def, type);

Can you take out the \n before PNK_CALL?

::: js/src/jit/shared/Assembler-shared.h
@@ +713,3 @@
>      AsmJSImm_PowD,
> +    AsmJSImm_ATan2D,
> +    AsmJSImm_Undefined

'undefined' had me confused (since there is a JS 'undefined').  How about AsmJSImm_Invalid?

::: js/src/jsmath.cpp
@@ +466,5 @@
>  }
>  
> +// Implementation of math_fround
> +bool
> +js::RoundFloat32(JSContext *cx, const Handle<Value> &v, float *out)

You can just take a 'Handle<Value> v', I think.

Also, for the comment, can you be a bit more specific and say:
Implements Math.fround (20.2.2.16) up to step 3

@@ +472,5 @@
> +    AssertArgumentsAreSane(cx, v);
> +    {
> +        js::SkipRoot root(cx, &v);
> +        js::MaybeCheckStackRoots(cx);
> +    }

I'd skip all this unless you think there is a good reason.

@@ +481,5 @@
> +    }
> +
> +    double slowOut;
> +    bool success = js::ToNumberSlow(cx, v, &slowOut);
> +    *out = static_cast<float>(slowOut);

ToNumber is JS_ALWAYS_INLINE with the v.isNumber() case, so I would just use ToNumber directly.

Comment 35

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1 - AsmJS nodes MIR / LIR / Codegen + fixed nits

Comment 36

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - Type System

Thanks for the quick reviews! Fixed most of the issues.

- Regarding IsFRoundableLiteral / ExtractFRoundableLiteral, I thought it would make more sense to test AND extract directly, as testing needs to extract the NumLit to test for its kind. Doesn't seem unreasonable.
- Not sure about the wording for some newly introduced error messages.

Comment 37

[Luke Wagner [:luke]]

Comment on attachment 8343411 [details] [diff] [review]
Part 2 - Type System

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

\o/

Before landing, could you verify that all the majors demos continue to validate?

::: js/src/jit/AsmJS.cpp
@@ +845,5 @@
>      return NumLit(NumLit::NegativeInt, Int32Value(i64));
>  }
>  
> +static bool
> +ExtractFRoundableLiteral(ParseNode *pn, NumLit *literal)

Nice improvement, but could the outparam be a double?

@@ +2075,5 @@
> +        if (!curBlock_)
> +            return NULL;
> +
> +        Value v;
> +        v.setDouble(double(f));

You can just pass a DoubleValue(f) inline below.

@@ +2077,5 @@
> +
> +        Value v;
> +        v.setDouble(double(f));
> +
> +        JS_ASSERT(v.isNumber()); // sanity check

Actually, this might fail if the float is a non-canonical NaN.  That's ok, though; this non-canonical NaN won't escape jit code w/o being canonicalized.

@@ +3042,5 @@
> +CheckGlobalVariableInitFloat32(ModuleCompiler &m, PropertyName *varName, ParseNode *initNode,
> +                               bool isConst)
> +{
> +    ParseNode *arg = NULL;
> +    if (!CheckFloat32Coercion(m, initNode, &arg, "caller in global initializer can only be fround"))

How about "call must be of the form fround(x)"?

@@ +4026,5 @@
> +{
> +    MDefinition *operand;
> +    Type operandType;
> +
> +    if (!CheckFRoundArg(f, callNode, &operand, &operandType, "coercion to float should use fround"))

Can you take out the \n before CheckFRoundArg?

@@ +4088,5 @@
> +      case AsmJSMathBuiltin_floor:  arity = 1; doubleCallee = AsmJSImm_FloorD; floatCallee = AsmJSImm_FloorF;    break;
> +      case AsmJSMathBuiltin_exp:    arity = 1; doubleCallee = AsmJSImm_ExpD; floatCallee = AsmJSImm_ExpF;        break;
> +      case AsmJSMathBuiltin_log:    arity = 1; doubleCallee = AsmJSImm_LogD; floatCallee = AsmJSImm_LogF;        break;
> +      case AsmJSMathBuiltin_pow:    arity = 2; doubleCallee = AsmJSImm_PowD; floatCallee = AsmJSImm_Invalid;   break;
> +      case AsmJSMathBuiltin_atan2:  arity = 2; doubleCallee = AsmJSImm_ATan2D; floatCallee = AsmJSImm_Invalid; break;

Can you align the 'break's at the end of these lines?

Comment 38

[Douglas Crosher [:dougc]]

Attachment: ARM backend support

This depends on the patch in bug 861785 which narrows Odin to use only the hard-float ABI.  Sorry I was loath to add all the soft-float ABI support for Odin-float32 when it was planned to be removed anyway.

Comment 39

[Luke Wagner [:luke]]

Comment on attachment 8343703 [details] [diff] [review]
ARM backend support

Sounds great, thanks!

Comment 40

[Douglas Crosher [:dougc]]

Attachment: ARM backend support

Rebased. Carrying forward r+.

Comment 41

[Douglas Crosher [:dougc]]

Attachment: Part 2 - Type System - rebases after bug 861785

A few minor changes will be needed to this patch to land this after bug 861785.

Comment 42

[Douglas Crosher [:dougc]]

Attachment: ARM backend support

Silence a compilation warning.

Comment 43

[Benjamin Bouvier [:bbouvier] (inactive)]

Finally! \o/ All tests passing locally, for x86 and x64. Douglas told me he tested on ARM, so everything should be alright.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a63e23e9b03b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a78c740ec5

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a63e23e9b03b
https://hg.mozilla.org/mozilla-central/rev/b2a78c740ec5