1052325 - OdinMonkey: make return coercions for Math functions call not mandatory

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

From luke's comment in the odin simd bug:

> If we were symmetric with the Math functions, we'd require callsite coercions,
> like:
> 
> +Math_sin(a);
> int32x4(int32x4_add(a, b));
> 
> which means wouldn't have this IsSimdOperationCall() case in
> CheckUncoercedCall, you'd just add another case to CheckCall().
> However, the above is pretty gnarly and totally unnecessary,
> given that the type of the operation is completely known from the name.
> But the same is true for Math standard library functions too
> (and this is something people occasionally complain about).
> So perhaps we could generalize the Math stdlib to be callable here also?
> The important thing is that SIMD is symmetric with the other Math stdlibs.
> If you agree, it seems like you could make the Math changes in an
> independent bug blocking this and then, in this patch, you'd just be adding a extra case.

I do agree with this. However, this implies giving default expected return types to functions that could return otherwise different types. For instance, Math.ceil can return double or float32, according to its return type coercion.

This modification wouldn't prevent these type specializations to happen. It only implies that if there is no return type coercion, we just consider the return type to be the default one. The only drawback is this implicit default value, as Explicit Is Better Than Implicit, but that probably makes sense for most Math functions.

I propose the default return coercion to be Signed for Math.imul, and Double for everything else. Is everybody fine with that?

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: AsmJS: make return coercion optional for standard math lib functions calls

So this implements what's been suggested above. In particular, Math.fround can't be
accessed through CheckMathBuiltinImplicitCall, as it is handled before. We
could actually remove the particular IsFloatCoercion call in
CheckUncoercedCall, by having Math.fround return RetType::Float by default, but that
would break symmetry with the SIMD type coercions checks (which would be
handled in CheckUncoercedCall, while the float type would be coerced in
CheckMathBuiltinCall). Opinions?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: AsmJS: make return coercion optional for standard math lib functions calls

Here is the way it works:

- There is a CheckImplicitMathBuiltinCall (suggestion of a better and
  not-too-long name will be appreciated :)), which is called in
CheckUncoercedCall.
- CheckCoercedCall (formerly known as CheckCall) calls into
  CheckCoercedMathBuiltinCall if the callee is a math function.
- CheckCoercedMathBuiltinCall calls into CheckUncoercedCall.

So I've done it, but there a few things which are unfortunate:
- A coerced call to Math.fround will need a conversion to happen after the
  call. If we don't want to loosen calls to other float32 math builtins (i.e.
floor and ceil, which would almost accept any return type), calls to Math.fround have
to be special-cased :/ (otherwise we could have e.g. var x=0;
x=abs(fround(42))|0)
- The way CheckCallArgs works makes it need to know ahead of time what will be
  the return type. In the patch, we check the first argument and consider it
will be the return type of the call, and then only check the second argument, if
there's one. That implies that CheckCallArgs can't be used, so logic from
CheckCallArgs is extracted manually and adds a bunch of boilerplate :/

A few pros, however:
- CheckIsMaybeDouble and CheckIsMaybeFloat aren't needed anymore.
- It allows to spare a few chars by math call (see tests): var x=fround(42); x=abs(x);

What's your opinion about it?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Renaming patch

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8472996 [details] [diff] [review]
AsmJS: make return coercion optional for standard math lib functions calls

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

I agree with your choice to 'inline' CheckCallArgs into CheckMathBuiltinCall.  I think the other parts can be simplified a bit.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +1696,5 @@
>  
> +    *global = m.lookupGlobal(callee->name());
> +    if (!*global)
> +        return false;
> +    return true;

I'd 'return !!*global;'

@@ +4033,5 @@
>      MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("return value of fround is ignored");
>  }
>  
>  static bool
> +CheckMathBuiltinImplicitCall(FunctionCompiler &f, ParseNode *callNode, AsmJSMathBuiltinFunction func,

I'd just leave it CheckMathBuiltinCall.

@@ +4071,5 @@
> +    MDefinition *firstArg;
> +    ParseNode *argNode = CallArgList(callNode);
> +    if (!CheckExpr(f, argNode, &firstArg, &firstType))
> +        return false;
> +    if (!firstType.isMaybeFloat() && !firstType.isMaybeDouble())

\n before second 'if'

@@ +4096,5 @@
> +        if (firstType.isMaybeDouble() && !secondType.isMaybeDouble())
> +            return f.fail(argNode, "both arguments to math builtin call should be the same type");
> +        if (firstType.isMaybeFloat() && !secondType.isMaybeFloat())
> +            return f.fail(argNode, "both arguments to math builtin call should be the same type");
> +        if (!f.passArg(secondArg, varType, &call))

I'd put a \n before the passArg.

@@ +4100,5 @@
> +        if (!f.passArg(secondArg, varType, &call))
> +            return false;
> +    }
> +
> +    f.finishCallArgs(&call);

I'd put a \n after the finishCallArgs.

@@ +4103,5 @@
> +
> +    f.finishCallArgs(&call);
> +    if (retType == RetType::Float && !f.builtinCall(floatCallee, call, MIRType_Float32, def))
> +        return false;
> +    if (retType == RetType::Double && !f.builtinCall(doubleCallee, call, MIRType_Double, def))

I think you can have a single:
  if (!f.builtinCall(..., retType.toMIRType())
     return false;

@@ +4124,5 @@
> +
> +    // Math builtin functions are all side-effects free, definition and return
> +    // types should be ignored by the caller.
> +    if (retType == RetType::Void)
> +        return true;

I'd avoid this special case and set *def to null in the switch I suggest below.  That'll make the math op dead code.  If this was expected to be common, it'd be nice to not even emit the IR, but I expect it'll be quite rare.

@@ +4127,5 @@
> +    if (retType == RetType::Void)
> +        return true;
> +
> +    MDefinition *operand;
> +    if (!CheckUncoercedCall(f, callNode, &operand, type))

With the float change mentioned below, can you just call CheckMathBuiltinCall directly here?

@@ +4136,5 @@
> +        *type = Type::Signed;
> +
> +    if (retType.toType() != *type)
> +        return f.failf(callNode, "return type of math function (%s) and use's type don't match",
> +                       type->toChars());

This part doesn't quite make sense to me: I should be able to write:
  +imul(1, 2)
With the new type rules: imul validates by itself as a signed, and + (the operator) accepts signed.

What I'd expect in place right here is a case analysis on the retType and, within each case, a case analysis on the math return type with the appropriate coericions applied.  The switch would also take care of setting *type to match the coercion, not the return type of the math builtin call.

@@ +4686,5 @@
> +    const ModuleCompiler::Global *global;
> +    if (IsCallToGlobal(f.m(), expr, &global) &&
> +        global->which() == ModuleCompiler::Global::MathBuiltinFunction)
> +    {
> +        return CheckMathBuiltinImplicitCall(f, expr, global->mathBuiltinFunction(), def, type);

From the spec:
  http://asmjs.org/spec/latest/#validatefloatcoercion-e
I think you don't need any special handling of fround.  Instead, you can just call CheckMathBuiltinCall(), that'll see it has fround and call CheckMathFRound, and that'll either take the "coerced call to float function" case or take the CheckFRoundArg case.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch with updated tests

Updated patch.

One thing worth to mention: there was before a discrepancy between the spec and Odin. The expression

fround(42)|0;

would get accepted by Odin, although the spec says |0 validates for call iff the actual return type of the call is a subtype of intish, which isn't the case here. I hope emscripten didn't use this implicit conversion (I've tested on massive's box2d-f32, it didn't raise any compilation error, so I guess it isn't the case, but wanted to mention it just in case it breaks demos).

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8478322 [details] [diff] [review]
Patch with updated tests

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

Beautiful!  I feel like this patch really clarifies the coerced/uncoerced call situation (previously restricted to fround).

::: js/src/asmjs/AsmJSValidate.cpp
@@ +3975,3 @@
>      MDefinition *operand;
> +    Type inputType;
> +    if (!CheckExpr(f, argNode, &operand, &inputType))

It's kindof nice when the MDefinition/Type/ParseNode have the same prefix.  How about argNode/Def/Type?

@@ +4051,5 @@
> +
> +        if (firstType.isMaybeDouble() && !secondType.isMaybeDouble())
> +            return f.fail(argNode, "both arguments to math builtin call should be the same type");
> +        if (firstType.isMaybeFloat() && !secondType.isMaybeFloat())
> +            return f.fail(argNode, "both arguments to math builtin call should be the same type");

I think you can write:
  if (!(secondType <= varType))
      return f.fail...

@@ +4060,5 @@
> +
> +    f.finishCallArgs(&call);
> +
> +    AsmJSImmKind callee = (retType == RetType::Double) ? doubleCallee : floatCallee;
> +    if (!f.builtinCall(callee, call, retType.toMIRType(), def))

I'd avoid creating retType altogether and just branch on 'argType' (which has toMIRType() and toType()).

@@ +4091,5 @@
> +          case Type::Unsigned:
> +            *def = f.unary<MAsmJSUnsignedToDouble>(operand);
> +            break;
> +          default:
> +            MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("unexpected math function return type");

I'm kinda reticent to add a 'default:' label to any switch statement since it avoids the warning you get when adding a type.  You could almost have CheckMathBuiltinCall have a RetType outparam, except for 'unsigned'.  What about adding a new tiny MathRetType that includes 'Unsigned'?  That way you could remove the default labels and Type::kind().  You'd also have a MathRetType::toType() that CheckUncoercedCall would use.

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

I'd move this function to below CheckMathBuiltinCall() (where the forward decl is).

@@ +4678,5 @@
>      JS_ASSERT(expr->isKind(PNK_CALL));
>  
> +    const ModuleCompiler::Global *global;
> +    if (IsCallToGlobal(f.m(), expr, &global) &&
> +        global->which() == ModuleCompiler::Global::MathBuiltinFunction)

I would not object to adding a Global::isMathFunction() which would allow this and the above 'if' statements to fit on a single line which is prettier.

::: js/src/jit-test/tests/asm.js/testMathLib.js
@@ +113,5 @@
> +assertEq(asmLink(asmCompile('glob', USE_ASM + 'var im=glob.Math.imul; function f(i) { i=i|0; i = im(i,i); return i|0 } return f'), this)(3), 9);
> +assertAsmTypeFail('glob', USE_ASM + 'var im=glob.Math.imul; function f(d) { d=+d; d = im(d, d) } return f');
> +assertAsmTypeFail('glob', USE_ASM + FROUND + 'var im=glob.Math.imul; function f(d) { d=fround(d); d = im(d, d) } return f');
> +
> +assertEq(asmLink(asmCompile('glob', USE_ASM + 'var abs=glob.Math.abs; function f(d) { d=d|0; d = abs(d|0); return +(d>>>0) } return f'), this)(-1), 1);

Can you add tests for using abs() in a context that requires 'unsigned' (like comparison with another unsigned) and then a test fails b/c it requires 'signed' (like comparison against a signed).

@@ +119,5 @@
> +assertEq(asmLink(asmCompile('glob', USE_ASM + FROUND + 'var abs=glob.Math.abs; function f(d) { d=fround(d); d = abs(d); return +d } return f'), this)(-1.5), 1.5);
> +
> +assertEq(asmLink(asmCompile('glob', USE_ASM + 'var sqrt=glob.Math.sqrt; function f(d) { d=+d; d = sqrt(d); return +d } return f'), this)(256), 16);
> +assertEq(asmLink(asmCompile('glob', USE_ASM + FROUND + 'var sqrt=glob.Math.sqrt; function f(d) { d=fround(d); d = sqrt(d); return +d } return f'), this)(13.37), Math.fround(Math.sqrt(Math.fround(13.37))));
> +assertAsmTypeFail('glob', USE_ASM + 'var sqrt=glob.Math.sqrt; function f(d) { d=d|0; d = sqrt(d) } return f');

It'd be good to separate out the arg type checking (sqrt(i|0)) from the return type checking (sqrt(1.0)|0).

@@ +141,4 @@
>  assertAsmTypeFail('glob', USE_ASM + 'var abs=glob.Math.abs; function f(d) { d=+d; abs(d)|0; } return f');
>  
> +assertEq(asmLink(asmCompile('glob', USE_ASM + 'var im=glob.Math.imul; function f(i) { i=i|0; var d=0.0; d = +im(i,i); return +d } return f'), this)(42), Math.imul(42, 42));
> +assertEq(asmLink(asmCompile('glob', USE_ASM + 'var abs=glob.Math.abs; function f(i) { i=i|0; var d=0.0; d = +abs(i|0); return +d } return f'), this)(-42), 42);

Can you also add tests for min/max?

Comment 7

[Alon Zakai (:azakai)]

I tested these patches on the emscripten test suite, looks like everything validates, so no issues on that front.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

I've fixed most of the nits and hit a bad bug when adding tests. With implicit coercion and float32, operations can be applied as float32 in asm.js and double in non-asm.js. Consider the following piece of code:

function f() { var x = fround(42); x = sqrt(x); }

With this patch, this code is valid asm.js. In asm.js, the float32 variant of sqrt is used, as the argument is a float32. In non-asm.js land, the double variant of sqrt is used. So this piece of code is equivalent to

x = Math.fround(Math.sqrt(Math.fround(x))); // in asm.js, resulting x is a float32
x = Math.sqrt(Math.fround(x)); // in non-asm.js, resulting x is a double

which have obviously different values (had a test fail on that!), resulting in differences of behavior between asm.js and non asm.js.

A solution could be the following: when the input is a float32, convert it to a double (MToDouble) and apply the double variant of the operation. This ensures it will behave as in the interpreter. Drawbacks:
1) float32 math operations will still need a return coercion
2) the double variant of the operation will be used, although the float32 version could be used.

2 could be avoided, thanks to GVN or to the ApplyTypes phase, but it creates a lot more work than needed. Maybe we could directly do pattern matching in CheckCoercedMathBuiltinCall, and replace operations of the form MToFloat32(Msomething(MToDouble(x))) to Msomething32, as the "theorem" states these two are actually equal?


(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8478322 [details] [diff] [review]
> Patch with updated tests
> 
> Review of attachment 8478322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +4051,5 @@
> > +
> > +        if (firstType.isMaybeDouble() && !secondType.isMaybeDouble())
> > +            return f.fail(argNode, "both arguments to math builtin call should be the same type");
> > +        if (firstType.isMaybeFloat() && !secondType.isMaybeFloat())
> > +            return f.fail(argNode, "both arguments to math builtin call should be the same type");
> 
> I think you can write:
>   if (!(secondType <= varType))
>       return f.fail...
Couldn't do that. operator<=(Type, VarType) uses precise types (e.g. isDouble rather than isMaybeDouble).

> 
> @@ +4060,5 @@
> > +
> > +    f.finishCallArgs(&call);
> > +
> > +    AsmJSImmKind callee = (retType == RetType::Double) ? doubleCallee : floatCallee;
> > +    if (!f.builtinCall(callee, call, retType.toMIRType(), def))
> 
> I'd avoid creating retType altogether and just branch on 'argType' (which
> has toMIRType() and toType()).
I've done that, and stored (varType.isMaybeDouble()) in a boolean called opIsDouble (as in CheckMathMinMax), so that all function calls can fit in a row.

I've addressed all other comments. Probably will flag again for review as changes are non-trivial.

Comment 9

[Luke Wagner [:luke]]

Nice find!  As discussed on IRC, we can have the Math stdlib functions return type 'floatish'.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch with updated tests v2

Re-asking for review, as there are now non-trivial changes (MathRetType, float/floatish).

So, with my previous comment taken into account: with this patch, return coercions are not mandatory anymore, *except for* float32 calls, as otherwise we'd have difference of behaviors between asm.js and normal js.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8479059 [details] [diff] [review]
Patch with updated tests v2

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +546,5 @@
>      bool operator==(RetType rhs) const { return which_ == rhs.which_; }
>      bool operator!=(RetType rhs) const { return which_ != rhs.which_; }
>  };
>  
> +// Represents the subset of RetType that can be used as a return type of a

Neither sub- (b/c unsigned/floatish) nor super- (b/c no void) set.  I'd just say "subset of Type"

@@ +3676,4 @@
>  
>      *def = f.mul(lhsDef, rhsDef, MIRType_Int32, MMul::Integer);
>      *type = Type::Signed;
> +    *retType = MathRetType(MathRetType::Signed);

As with Type, I'd make MathRetType's ctor MOZ_IMPLICIT so that you can just write '*type = MathRetType::Signed' (here and below).

@@ +4031,5 @@
>  }
>  
>  static bool
>  CheckMathBuiltinCall(FunctionCompiler &f, ParseNode *callNode, AsmJSMathBuiltinFunction func,
> +                     MDefinition **def, Type *type, MathRetType *mathRetType)

IIUC, you don't need both 'type' and 'mathRetType'; both callers of CheckMathBuiltinCall ignore the 'type' outparam value so I think you can just have a 'type' outparam which is a MathRetType*

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1 - renaming

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - make return coercion optional

addressed reviewer's feedback, carrying forward r+ from luke

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7989d0848f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/684b02de6a31

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4f7989d0848f
https://hg.mozilla.org/mozilla-central/rev/684b02de6a31