Closed
Bug 1052325
Opened 7 years ago
Closed 7 years ago
OdinMonkey: make return coercions for Math functions call not mandatory
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(2 files, 5 obsolete files)
4.55 KB,
patch

bbouvier
:
review+

Details  Diff  Splinter Review 
44.27 KB,
patch

bbouvier
:
review+

Details  Diff  Splinter Review 
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?
Assignee  
Comment 1•7 years ago


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?
Attachment #8471459 
Flags: review?(luke)
Assignee  
Comment 2•7 years ago


Here is the way it works:  There is a CheckImplicitMathBuiltinCall (suggestion of a better and nottoolong 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 specialcased :/ (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?
Attachment #8472996 
Flags: feedback?(luke)
Assignee  
Updated•7 years ago

Attachment #8471459 
Attachment is obsolete: true
Attachment #8471459 
Flags: review?(luke)
Assignee  
Comment 3•7 years ago


Attachment #8473000 
Flags: review?(luke)
Updated•7 years ago

Attachment #8473000 
Flags: review?(luke) → review+
Comment 4•7 years ago


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 sideeffects 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/#validatefloatcoercione 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.
Attachment #8472996 
Flags: feedback?(luke) → feedback+
Assignee  
Comment 5•7 years ago


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 box2df32, 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).
Attachment #8472996 
Attachment is obsolete: true
Attachment #8478322 
Flags: review?(luke)
Comment 6•7 years ago


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/jittest/tests/asm.js/testMathLib.js @@ +113,5 @@ > +assertEq(asmLink(asmCompile('glob', USE_ASM + 'var im=glob.Math.imul; function f(i) { i=i0; i = im(i,i); return i0 } 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=d0; d = abs(d0); 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=d0; d = sqrt(d) } return f'); It'd be good to separate out the arg type checking (sqrt(i0)) 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=i0; 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=i0; var d=0.0; d = +abs(i0); return +d } return f'), this)(42), 42); Can you also add tests for min/max?
Attachment #8478322 
Flags: review?(luke) → review+
Comment 7•7 years ago


I tested these patches on the emscripten test suite, looks like everything validates, so no issues on that front.
Assignee  
Comment 8•7 years ago


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 nonasm.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 nonasm.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 nonasm.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 nontrivial.
Flags: needinfo?(luke)
Comment 9•7 years ago


Nice find! As discussed on IRC, we can have the Math stdlib functions return type 'floatish'.
Flags: needinfo?(luke)
Assignee  
Comment 10•7 years ago


Reasking for review, as there are now nontrivial 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.
Attachment #8478322 
Attachment is obsolete: true
Attachment #8479059 
Flags: review?(luke)
Comment 11•7 years ago


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*
Attachment #8479059 
Flags: review?(luke) → review+
Assignee  
Comment 12•7 years ago


Attachment #8473000 
Attachment is obsolete: true
Attachment #8479726 
Flags: review+
Assignee  
Comment 13•7 years ago


addressed reviewer's feedback, carrying forward r+ from luke
Attachment #8479059 
Attachment is obsolete: true
Attachment #8479727 
Flags: review+
Assignee  
Comment 14•7 years ago


remote: https://hg.mozilla.org/integration/mozillainbound/rev/4f7989d0848f remote: https://hg.mozilla.org/integration/mozillainbound/rev/684b02de6a31
https://hg.mozilla.org/mozillacentral/rev/4f7989d0848f https://hg.mozilla.org/mozillacentral/rev/684b02de6a31
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution:  → FIXED
Target Milestone:  → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•