1067502 - Odin: Be less coercive for float32x4 initializers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Currently, Odin accepts the following types as inputs for a float32x4 constructor or .splat argument: float/float?/floatish, double/double?, signed/unsigned. Although fround() accepts all of these, it makes sense only for fround as it is an explicit coercion; moreover, this breaks symmetry with int32x4 which can only accept intish. So float32x4 should only accept floatish. Same applies for float32x4.splat.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Be less coercive for float32x4 ctor and splat inputs

This patch implements two things:
- fix generation of coerced float32s. For instance, if you had return
  fround(fround(42)), it would generate a MToFloat32(MConstant(Int32Value(42)));
- fix generation of SIMD literals the same way: var x=f32x4(1,2,3,4);
  x=f32x4(1,2,3,5); // Odin would generate a MSimdValueX4, not a
  MSimdConstant.
- restrict float32x4 ctor and splat to only receive floatish.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8489523 [details] [diff] [review]
Be less coercive for float32x4 ctor and splat inputs

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +4635,5 @@
> +          case AsmJSNumLit::Float32x4:
> +          case AsmJSNumLit::OutOfRangeInt:
> +            break;
> +        }
> +    }

Since I wouldn't expect fround(fround(42)) to show up in practice and since, iiuc, we're correct without this special case, I'd rather leave it out.

I was thinking, though, you *almost* couldn't hit this case here or in CheckSimdCtorCall because CheckExpr first checks IsNumericLiteral; the problem is that CheckCoercedCall circumvents CheckExpr (since it wants to propagate the return type).  Instead, can we have a single IsNumericLiteral/CheckNumericLiteral at the head of CheckCoercedCall()?

@@ +4906,5 @@
>  
>        case AsmJSSimdOperation_splat: {
>          DefinitionVector defs;
> +        Type coerciveType = retType.simdToCoercedScalarType();
> +        if (!CheckSimdCallArgs(f, call, 1, CheckArgIsSubtypeOf(coerciveType), &defs))

Maybe s/coerciveType/formalType/?

@@ +4950,2 @@
>      unsigned length = SimdTypeToLength(simdType);
> +    Type coercedType = retType.simdToCoercedScalarType();

Also formalType?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Be less coercive for float32x4 ctor and splat inputs

Wow, much simpler, I should have used that from the beginning.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8490027 [details] [diff] [review]
Be less coercive for float32x4 ctor and splat inputs

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

Ah, beautiful.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +4923,1 @@
>      unsigned length = SimdTypeToLength(simdType);

Can you take out the \n between these decls?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Yes, I did. Good news, the two SIMD demos out there don't need any change to their code \o/

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb4a65fdd66

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Sorry about that, I forgot to test locally all of asm.js test subdir, and float32 tests aren't passing. Taking time to investigate, backing it out the previous patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/31f84a923a18

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Less coercive but still correct

Changes aren't trivial, so requesting review again. I am really sorry not to have run all asm.js jit-tests, I don't understand how this went off my head :/
The issue was that in CheckCoercedCall, we need to also take the coercion into account (it wasn't done, so +fround(42) would just state that the expression was typed as float).

To prevent this from happening again, I've also added tests for that in testSIMD.js (+int32x4(1,2,3,4), fround(int32x4(1,2,3,4)), int32x4(1,2,3,4)|0, void int32x4(1,2,3,4), float32x4(int32x4(1,2,3,4)) and so on).

Comment 8

[Luke Wagner [:luke]]

D'oh!  With this patch, we'll have three CheckCoerced* functions.  In effect, they all do the same thing: call something that doesn't care about the RetType, and then apply the coercion implied by RetType.  In theory, it doesn't matter what we called, the RetType-implied coercion is the same.  So how about factoring out a CoerceResult() function that takes the result of Math/Simd/Literal and applies any necessary coercion.  This could be called directly from CheckCoercedCall (after calling the uncoerced Check* function) so we didn't need these three CheckCoerced* functions.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2, refactoring

This refactoring works great. I've kept CheckCoercedMathBuiltin / CheckCoercedSimdCall, as this would make the switch in CheckCoercedCall less beautiful imo.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1, be less coercive and correct

Interdiff:
- removed the argDef argument in the Check* function closures

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8490890 [details] [diff] [review]
Part 2, refactoring

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

Great, thanks!

::: js/src/asmjs/AsmJSValidate.cpp
@@ +4961,5 @@
>  }
>  
>  static bool
> +CoerceResult(FunctionCompiler &f, ParseNode *expr, RetType expected, Type resultType,
> +             MDefinition *result, MDefinition **def, Type *type)

Since 'type' follows 'def', could you have 'resultType' follow 'result' in the parameter list?

@@ +4965,5 @@
> +             MDefinition *result, MDefinition **def, Type *type)
> +{
> +    switch (expected.which()) {
> +      case RetType::Void: {
> +        *def = nullptr;

Should probably still assign *type = Type::Void.

@@ +4970,5 @@
> +        return true;
> +      }
> +
> +      case RetType::Signed: {
> +        if (!resultType.isSigned() && !resultType.isUnsigned())

Couldn't this accept intish (since it is necessarily feeding into ToInt32())?  (There may not be any stdlib functions return this, but CheckBitwise checks isIntish.)

@@ +4992,5 @@
> +            *def = f.unary<MAsmJSUnsignedToDouble>(result);
> +            return true;
> +        }
> +        return f.failf(expr, "%s is not a subtype of double?, float?, signed or unsigned",
> +                       resultType.toChars());

I think you can also replace the tail of CheckPos with
  return CoerceResult(f, operand, RetType::Double, operandDef, operandType, def, type);
In theory you could do this to the others, but CheckPos is the only one with interesting contents.

@@ +5028,5 @@
>                              RetType retType, MDefinition **def, Type *type)
>  {
>      MDefinition *operand;
>      MathRetType actualRetType;
>      if (!CheckMathBuiltinCall(f, callNode, func, &operand, &actualRetType))

Can you rename 'operand' to 'result' and 'actualRetType' to 'resultType'?

@@ +5057,5 @@
>      JS_CHECK_RECURSION_DONT_REPORT(f.cx(), return f.m().failOverRecursed());
>  
> +    if (IsNumericLiteral(f.m(), call)) {
> +        AsmJSNumLit literal = ExtractNumericLiteral(f.m(), call);
> +        *def = f.constant(literal);

Instead of mutating *def (which definitely is clobbered by CoerceResult), could have have 'MDefinition *result = f.constan(literal);'?

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

I've addressed all comments:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/78c4acde1ac5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e797bfdad38

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/78c4acde1ac5
https://hg.mozilla.org/mozilla-central/rev/8e797bfdad38