1244889 - OdinMonkey - SIMD: add support for Uint32x4

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

Currently, asm.js supports SIMD.Int32x4, but not SIMD.Uint32x4.

We anticipate that WebAssembly will not adopt the signed/unsigned SIMD integer types, but instead add signed/unsigned compare and float conversion operations, just like they do for the scalar I32 and I64 types. This complicates things a little since we don't want to add U32x4 to ValType and ExprType in asmjs/WasmTypes, for example.

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

OK, this requires some refactoring in AsmJS.cpp. The asm.js and wasm type systems are diverging--asm.js doesn't have I64, and wasm doesn't distinguish signed/unsigned SIMD types.

I am going to reduce the dependency on wasm::ValType and wasm::ExprType in the AsmJS.cpp code. I will use a canonical subset of the Type::Which enumeration instead.

This will only require changes in AsmJS.cpp. We will still use ValType and ExprType to register global variables and function signatures with the wasm code generator.

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91fd8d4967b0

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Remove toMIRType() and simdType() methods.

These methods on the asm.js Type class were unused.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Remove toMIRType() and simdType() methods.

These methods on the asm.js Type class were unused.

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Define a canonical subset of Type::Which.

This subset of the asm.js types corresponds to the Void type and all the
coercion targets. It corresponds to wasm::ExprType, except:

- asm.js does not support I64.
- asm.js distinguishes signed/unsigned SIMD types, wasm does not.

The function Type::expr() maps a type into the canonical set. Type::isExprType()
checks for membership.

This subset of Type::Which will be used to replace wasm::ValType and
wasm::ExprType in the asm.js type checking code. This makes it possible to
type-check unsigned SIMD types correctly.

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Reduce dependency on wasm::ValType.

The WebAssembly and the asm.js type systems are diverging. In particular,
WebAssembly will not distinguish signed/unsigned SIMD types, while asm.js must
since it is compatible with SIMD.js. The wasm::ValType enum also has an I64
which is not supported by asm.js

Reduce OdinMonkey's dependency on wasm::ValType by returning a canonical 'class
Type' instead from these functions:

- IsCoercionCall()
- CheckTypeAnnotation()
- CheckArgumentType()

Avoid wasm::ValType for asm.js locals. We want to allow signed/unsigned SIMD
locals.

Remove NumLit::type().

Comment 7

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Reduce dependency on wasm::ExprType.

When validating function calls and coercions, use a canonical Type instead of a
wasm::ExprType to express the desired return type.

This will make it possible to distinguish signed/unsigned SIMD types during
validation.

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Fix CheckCoercionArg and CheckReturnType.

These two functions can also take a Type argument instead of ValType/ExprType.

This makes it possible to avoid some explicit SIMD type enumerations, and the
Type <= ValType subtyping test is no longer needed.

Comment 9

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Remove SimdTypeToLength().

Use the existing GetSimdLanes() defined in SIMD.h.

Comment 10

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Remove trivial SIMD NewAsmJS factories.

The MSimd*::NewAsmJS() factory functions are not longer needed since the normal
MSimd*::New() functions also require the SIMD arguments to be unboxed since
bug 1244254.

Remove the trivial NewAsmJS functions here. The remaining SIMD NewAsmJS
functions will be removed in the next commit.

Comment 11

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Amend wasm opcodes for unsigned I32x4 operations.

The wasm types don't have signed/unsigned SIMD integers. Add enumerators for
those unsigned simd operations that behave differently than their signed I32x4
counterparts.

Implement wasm code generation for unsigned SIMD types.

Split the convertSimd() function into two variants for conversions and bitcasts
respectively. Add a SimdSign argument to the conversion variants.

Add a SimdSign argument to the extractSimdElement() function. This is not
strictly needed for I32x4 since wasm uses I32 for both signed and unsigned ints.
It will be required for the I8x16 and I16x8 types, though, because the lane
value needs to be extended to an I32.

Split out the binarySimdComp function for binary comparisons, and thread
through a SimdSign argument.

Add a SimdSign to EmitSimdOp() and thread it through where needed.

Comment 12

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Handle all SIMD types in js::SimdTypeToName.

Also move IsSimdTypeName() into builtin/SIMD.cpp and handle all SIMD type names
there too.

Use IsSimdValidOperationType(simdType, SimdOperation::Constructor) to check for
supported SIMD types in asm.js instead of depending on IsSimdTypeName() to only
return true on supported types.

Comment 13

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Add support for Uint32x4 as an asm.js type.

Add Uint32x4 to NumLit and Type classes.

Reject unsigned SIMD types in function signatures to avoid confusion when
interoperating with WebAssembly.

Comment 14

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Fix Float32x4toUint32x4 for asm.js.

The machine code needs to branch to wasm::JumpTarget::ConversionError instead of
bailing out.

Comment 15

[Jakob Stoklund Olesen [:jolesen]]

I think something still needs to be done about global variables. We should probably disallow unsigned SIMD types for global variables, like we do for function signatures in these patches.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718464 [details] [diff] [review]
Remove toMIRType() and simdType() methods.

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

Nice! would r+ again.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718466 [details] [diff] [review]
Remove toMIRType() and simdType() methods.

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

And did r+ again (same patch as previous one).

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718472 [details] [diff] [review]
Remove SimdTypeToLength().

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

Nice.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718473 [details] [diff] [review]
Remove trivial SIMD NewAsmJS factories.

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

Cool.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718475 [details] [diff] [review]
Handle all SIMD types in js::SimdTypeToName.

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

Nice for the refactoring parts, but I don't understand something in AsmJS.cpp, so I'll wait for your answer first.

::: js/src/asmjs/AsmJS.cpp
@@ +3182,3 @@
>          return m.failName(initNode, "'%s' is not a standard SIMD type", field);
> +    if (!IsSimdValidOperationType(simdType, SimdOperation::Constructor))
> +        return m.failName(initNode, "'%s' is not a supported SIMD type", field);

Why do we need to add this check, and the Constructor case in IsSimdValidOperationType? It seems your uint32x4 patch does return true even for uint32x4, and these changes could be reverted.

::: js/src/builtin/SIMD.cpp
@@ +85,5 @@
> +PropertyName*
> +js::SimdTypeToName(const JSAtomState& atoms, SimdType type)
> +{
> +    switch (type) {
> +#define CASE(_) case SimdType::_: return atoms._;

Can you give a significant name to the macro argument, please? Even "T" would be a better name, but let's get to the next level and call it "NAME" or "TYPE", or whatever you think is more descriptive than "_".

@@ +86,5 @@
> +js::SimdTypeToName(const JSAtomState& atoms, SimdType type)
> +{
> +    switch (type) {
> +#define CASE(_) case SimdType::_: return atoms._;
> +FOR_EACH_SIMD(CASE)

nit: please align FOR_EACH_SIMD(CASE) as if it were in the switch (2 or 4 spaces indent, this doesn't matter much in this case)(pun not intended).

@@ +88,5 @@
> +    switch (type) {
> +#define CASE(_) case SimdType::_: return atoms._;
> +FOR_EACH_SIMD(CASE)
> +#undef CASE
> +      default:                  break;

I think we can even delete the "default" case, as all cases should be explicitly handled? If so, let's also change the MAKE_COMPILER_ASSUME_BLABLABLA to MOZ_CRASH.

@@ +96,5 @@
> +
> +bool
> +js::IsSimdTypeName(const JSAtomState& atoms, const PropertyName* name, SimdType* type)
> +{
> +#define CASE(_) if (name == atoms._) {  \

ditto

@@ +100,5 @@
> +#define CASE(_) if (name == atoms._) {  \
> +                    *type = SimdType::_;      \
> +                    return true;              \
> +                }
> +FOR_EACH_SIMD(CASE)

ditto

::: js/src/builtin/SIMD.h
@@ +1107,5 @@
> +PropertyName* SimdTypeToName(const JSAtomState& atoms, SimdType type);
> +
> +// Check if name is the well known name of a SIMD type.
> +// Returns true and sets *type if name is known.
> +// Returns false if name is not the name of a SIMD type.

Could just be "returns true and sets *type iff the name is known".

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718477 [details] [diff] [review]
Fix Float32x4toUint32x4 for asm.js.

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

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +2411,5 @@
>      masm.vmovmskps(scratch, temp);
>      masm.cmp32(temp, Imm32(15));
> +
> +    if (gen->compilingAsmJS())
> +        masm.j(Assembler::NotEqual, &onConversionError);

It seems this could just be masm.j(Assembler::NotEqual, wasm::JumpTarget::ConversionError), or the masm.jmp variant?

@@ +2451,5 @@
>      masm.cmp32(temp, Imm32(0));
> +
> +    if (gen->compilingAsmJS()) {
> +        masm.j(Assembler::NotEqual, &onConversionError);
> +        masm.bindLater(&onConversionError, wasm::JumpTarget::ConversionError);

Ditto? If so, you could remove the {} in the then/else bodies and get rid of the label.

Comment 22

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #20)
> Comment on attachment 8718475 [details] [diff] [review]
> Handle all SIMD types in js::SimdTypeToName.
> 
> Review of attachment 8718475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice for the refactoring parts, but I don't understand something in
> AsmJS.cpp, so I'll wait for your answer first.
> 
> ::: js/src/asmjs/AsmJS.cpp
> @@ +3182,3 @@
> >          return m.failName(initNode, "'%s' is not a standard SIMD type", field);
> > +    if (!IsSimdValidOperationType(simdType, SimdOperation::Constructor))
> > +        return m.failName(initNode, "'%s' is not a supported SIMD type", field);
> 
> Why do we need to add this check, and the Constructor case in
> IsSimdValidOperationType? It seems your uint32x4 patch does return true even
> for uint32x4, and these changes could be reverted.

Without this check, we would attempt to compile something like SIMD.Uint8x16(a, b, ...) and trigger all sorts of assertions. Before this patch, SimdTypeToName would filter that constructor call out. Now it doesn't, so we have to do it explicitly.

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718475 [details] [diff] [review]
Handle all SIMD types in js::SimdTypeToName.

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

Makes sense.

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718474 [details] [diff] [review]
Amend wasm opcodes for unsigned I32x4 operations.

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

::: js/src/asmjs/AsmJS.cpp
@@ +2570,4 @@
>        case SimdType::Int32x4: {
> +        // Bitcasts Uint32x4 <--> Int32x4 become noops.
> +        if (op == SimdOperation::Fn_fromUint32x4Bits)
> +            return Expr::Id;

nit:: can you put the return on the same line as the if, to be symmetric with the switch above, please?

::: js/src/asmjs/WasmBinary.h
@@ +286,5 @@
> +    I32x4lessThanU,
> +    I32x4lessThanOrEqualU,
> +    I32x4greaterThanU,
> +    I32x4greaterThanOrEqualU,
> +    I32x4fromFloat32x4U,

These names can too easily be confounded with the non unsigned operators. Can you expand the U to Unsigned, please? Alternatives that would work for me too: name these operators U32x4[op], or I32x4Unsigned[op].

::: js/src/asmjs/WasmIonCompile.cpp
@@ +278,5 @@
> +    {
> +        if (inDeadCode())
> +            return nullptr;
> +
> +        MOZ_ASSERT(lhs->type() == lhs->type());

tautological assert, did you mean rhs->type() on one side?

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718468 [details] [diff] [review]
Define a canonical subset of Type::Which.

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

Per discussion on IRC with luke, switching reviewer.

Comment 26

[Luke Wagner [:luke]]

Comment on attachment 8718474 [details] [diff] [review]
Amend wasm opcodes for unsigned I32x4 operations.

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

::: js/src/asmjs/WasmBinary.h
@@ +286,5 @@
> +    I32x4lessThanU,
> +    I32x4lessThanOrEqualU,
> +    I32x4greaterThanU,
> +    I32x4greaterThanOrEqualU,
> +    I32x4fromFloat32x4U,

If you look at the other opcode names, though, it is the wasm op naming convention to have a trailing U or S for Unsigned/Signed.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 8718476 [details] [diff] [review]
Add support for Uint32x4 as an asm.js type.

Stealing with permission.

Comment 28

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Remove toMIRType() and simdType() methods.

Rebased.

Comment 29

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Rename Type:isVarType() out of the way.

This function is only used by CheckIsVarType which is actually used for
checking function arguments.

Rename isVarType -> isArgType and CheckIsVarType -> CheckIsArgType to better
match the usage.

This frees up the concept of a "var type" for the following patch.

Comment 30

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Define a canonical subset of Type::Which.

Rebased, renamed functions to use the 'VarType' concept.

Comment 31

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Reduce dependency on wasm::ValType.

Rebased, renamed VarType functions.

Comment 32

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Reduce dependency on wasm::ExprType.

Rebased, use VarType functions.

Comment 33

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Fix CheckCoercionArg and CheckReturnType.

Rebased, renamed functions to VarType.

Comment 34

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Remove SimdTypeToLength().

Rebased.

Comment 35

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Remove trivial SIMD NewAsmJS factories.

Rebased.

Comment 36

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Amend wasm opcodes for unsigned I32x4 operations.

- Fixed formatting nits.
- Deleted tautological assertion. Turns out AddLegalized already has the same
  one.
- Did NOT change naming of opcode enumerators.

Comment 37

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Handle all SIMD types in js::SimdTypeToName.

- Added a comment before the call to IsSimdValidOperation(..., Constructor).
- Fixed nits in FOR_EACH_SIMD usage.
- Fixed comment in SIMD.

Comment 38

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Add support for Uint32x4 as an asm.js type.

Rebased.

Comment 39

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Fix Float32x4toUint32x4 for asm.js.

Simplify, don't use a label.

Comment 40

[Luke Wagner [:luke]]

Comment on attachment 8720484 [details] [diff] [review]
Rename Type:isVarType() out of the way.

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

::: js/src/asmjs/AsmJS.cpp
@@ +1249,4 @@
>          return isInt() || isFloat() || isDouble() || isSimd();
>      }
>  
>      ValType checkedValueType() const {

(If this doesn't go away in a separate patch, perhaps rename to checkedArgType.)

Comment 41

[Jakob Stoklund Olesen [:jolesen]]

Still TBD: Stricter type-checking of global variables. I'll attach a final patch that disallows unsigned SIMD types in globals.

Comment 42

[Luke Wagner [:luke]]

Comment on attachment 8720486 [details] [diff] [review]
Define a canonical subset of Type::Which.

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

::: js/src/asmjs/AsmJS.cpp
@@ +1065,5 @@
> +// "VarTypes" is a canonical subset of types representing the coercion targets:
> +// Int, Float, Double, and the SIMD types. This is almost equivalent to
> +// wasm::ValType, except the integer SIMD types have signed/unsigned variants.
> +//
> +// Void is also part of the vartypes which then maps to wasm::ExprType.

Drat!  This kinda makes "var type" a bad name.  As discussed on IRC, let's use "canonical type" where "canonical" means exactly what we choose it to mean (what you describe in this comment).

@@ +1149,5 @@
>      }
>  
> +    // Map |t| to one of the canonical vartype representations of a
> +    // wasm::ExprType.
> +    static Type var(Type t) {

So this would be 'canonicalize'

@@ +1298,5 @@
>      }
>  
> +    // Check if this is one of the canonical vartype representations of a
> +    // wasm::ExprType. See Type::var().
> +    bool isVarType() const {

How about isCanonical()?

@@ +1311,5 @@
> +        }
> +    }
> +
> +    // Check if this is a canonical representation of a wasm::ValType.
> +    bool isNonVoidVarType() const {

and isNonVoidCanonical()

@@ +1316,5 @@
> +        return !isVoid() && isVarType();
> +    }
> +
> +    // Convert this canonical type to a wasm::ExprType.
> +    ExprType toExprType() const {

How about canonicalToExprType() so we're stating the assumption that you've canonialized already in the name.

@@ +1330,5 @@
> +        }
> +    }
> +
> +    // Convert this canonical type to a wasm::ValType.
> +    ValType toValType() const {

and canonicalToValType()

Comment 43

[Luke Wagner [:luke]]

Comment on attachment 8720487 [details] [diff] [review]
Reduce dependency on wasm::ValType.

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

::: js/src/asmjs/AsmJS.cpp
@@ +1828,5 @@
>      }
>      bool addGlobalVarInit(PropertyName* var, const NumLit& lit, bool isConst) {
>          uint32_t index;
> +        Type litType = Type::lit(lit);
> +        Type varType = Type::var(litType);

Don't forget to update variable names (e.g., here to canonicalType).

@@ +2600,3 @@
>          unsigned slot;
> +        Local(Type t, unsigned slot) : type(t), slot(slot) {
> +            MOZ_ASSERT(type.isNonVoidVarType(), "Type must be canonical");

With renaming won't need string explanation to MOZ_ASSERT.

Comment 44

[Luke Wagner [:luke]]

Comment on attachment 8720488 [details] [diff] [review]
Reduce dependency on wasm::ExprType.

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

::: js/src/asmjs/AsmJS.cpp
@@ +4364,5 @@
>          return false;
>  
>      ModuleValidator::Func* callee;
> +    if (!CheckFunctionSignature(f.m(), callNode, Sig(Move(args), ret.toExprType()), calleeName,
> +                                &callee))

Unfortunately multi-line condition means you have to put { } around the body which I find super ugly so I usually go to lengths to avoid.  In this case, I'd pull out the Sig temporary into it's own named local with \n before and after (which I did for the same reason in CheckFuncPtrCall :).

@@ +4494,5 @@
>      if (!CheckCallArgs<CheckIsExternType>(f, callNode, &args))
>          return false;
>  
>      uint32_t importIndex;
> +    if (!f.m().declareImport(calleeName, Sig(Move(args), ret.toExprType()), ffiIndex,

Same comment about multiline and pulling out 'sig'.

@@ +5364,5 @@
>              f.encoder().patchExpr(patchAt, Expr::F64ConvertUI32);
>          else
>              return f.failf(expr, "%s is not a subtype of double?, float?, signed or unsigned", actual.toChars());
>          break;
> +      default:

Can you explicitly name the 3 SIMD cases (again, trying to avoid 'default' unless it's obnoxious otherwise).

Comment 45

[Luke Wagner [:luke]]

Comment on attachment 8720489 [details] [diff] [review]
Fix CheckCoercionArg and CheckReturnType.

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

::: js/src/asmjs/AsmJS.cpp
@@ +6424,5 @@
>      Type type;
>      if (!CheckExpr(f, expr, &type))
>          return false;
>  
> +    if (!(type.isSigned() || type.isFloat() || type.isDouble() || type.isSimd() || type.isVoid()))

How about creating a Type::isRetType (put right below isArgType for symmetry)?

Comment 46

[Luke Wagner [:luke]]

Comment on attachment 8720495 [details] [diff] [review]
Add support for Uint32x4 as an asm.js type.

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

Great job with this whole stack!

::: js/src/asmjs/AsmJS.cpp
@@ +1098,5 @@
>          }
>          MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("bad SimdType");
>      }
>  
>      static Type var(ValType t) {

Could you rename to globalVar()?

@@ +3329,5 @@
>      ParseNode* coercedExpr;
>      if (!CheckTypeAnnotation(f.m(), coercionNode, type, &coercedExpr))
>          return false;
>  
> +    if (type->isUnsignedSimd())

How about using isArgType() to do this test (assuming the change recommended below which makes isArgType return false for unsigned SIMD)?

@@ +4361,5 @@
>  CheckIsArgType(FunctionValidator& f, ParseNode* argNode, Type type)
>  {
>      if (!type.isArgType())
>          return f.failf(argNode, "%s is not a subtype of int, float or double", type.toChars());
> +    if (type.isUnsignedSimd())

Can instead update isArgType() so it's false for unsigned SIMD (and update the fail text)?

@@ +6466,5 @@
>      if (!(type.isSigned() || type.isFloat() || type.isDouble() || type.isSimd() || type.isVoid()))
>          return f.failf(expr, "%s is not a valid return type", type.toChars());
>  
> +    // We don't allow unsigned SIMD types in function signatures.
> +    if (type.isUnsignedSimd())

Assuming the isRetType suggested in my comment on the last patch, could you make isRetType() return false for isUnsignedSimd() instead?

Comment 47

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Disallow unsigned SIMD types for global variables.

Treat global variables the same as function arguments and return values: No
unsigned SIMD types allowed. This simplifies interoperation with WebAssembly.

Fold CheckGlobalVariableImportExpr into its single caller.

Rename Type::var(ValType) to 'arg'. This function is now only used to produce
error messages.

Comment 48

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b43c88fd7889

Comment 49

[Luke Wagner [:luke]]

Comment on attachment 8720568 [details] [diff] [review]
Disallow unsigned SIMD types for global variables.

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

::: js/src/asmjs/AsmJS.cpp
@@ +1104,5 @@
>          MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("bad SimdType");
>      }
>  
> +    // Convert a ValType representing a function argument to a canonical Type.
> +    static Type arg(ValType t) {

How about moving this out of class Type and into a static function named `ToChars` function right before the function that uses it?

@@ +2926,5 @@
>  {
>      NumLit lit = ExtractNumericLiteral(m, initNode);
>      if (!lit.valid())
>          return m.fail(initNode, "global initializer is out of representable integer range");
> +    if (lit.isUnsignedSimd())

In the spirit of isArgType/isRetType, how about giving Type an isGlobalVarType() and then testing lit.type().isGlobalVarType()?

@@ +2977,4 @@
>      if (!coercedExpr->isKind(PNK_DOT))
>          return m.failName(coercedExpr, "invalid import expression for global '%s'", varName);
>  
> +    if (coerceTo.isUnsignedSimd())

You could use isGlobalVarType() here too.

Comment 50

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Luke Wagner [:luke] from comment #49)
> Comment on attachment 8720568 [details] [diff] [review]
> Disallow unsigned SIMD types for global variables.
> 
> Review of attachment 8720568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/asmjs/AsmJS.cpp
> @@ +1104,5 @@
> >          MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("bad SimdType");
> >      }
> >  
> > +    // Convert a ValType representing a function argument to a canonical Type.
> > +    static Type arg(ValType t) {
> 
> How about moving this out of class Type and into a static function named
> `ToChars` function right before the function that uses it?

It looks like both Type::arg(ValType) and Type::ret(ExprType) are only used for diagnostics.

How about putting a ToChars(ValType/ExprType) in WasmTypes.h? It seems like it could be useful elsewhere too.

Comment 51

[Luke Wagner [:luke]]

There's actually already a ToCString(ExprType) in Wasm.cpp, so maybe just move it up to WasmTypes.h (static inline function below ToMIRType).  Now technically this will print "i32" instead of "int" but I think that's fine.

Comment 52

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b1586a8e404

Comment 53

[Jakob Stoklund Olesen [:jolesen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e82a5bd3381e6609b4222ba358d3beec14140555
Bug 1244889 - Remove toMIRType() and simdType() methods. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/5498464cedbfcf4f50628f48f732ba6c351f670e
Bug 1244889 - Rename Type:isVarType() out of the way. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee4b5764a109a6e5bb6714249cff9079d8c2bc9b
Bug 1244889 - Define a canonical subset of Type::Which. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/66b1403ab173c9fb871a867d626cc8b2b27cf7ed
Bug 1244889 - Reduce dependency on wasm::ValType. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/3577a7648ce7e23431ca99b3b7839ef6d92079e7
Bug 1244889 - Reduce dependency on wasm::ExprType. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/70b848b3abc6010e4f0171d14ed4a4750ddda842
Bug 1244889 - Fix CheckCoercionArg and CheckReturnType. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/746c90d31e354cbb96ed2a5f2d4823078b057428
Bug 1244889 - Remove SimdTypeToLength(). r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/16203ccd8f80055ac71bcd4ccbb1a247289ef107
Bug 1244889 - Remove trivial SIMD NewAsmJS factories. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdf89af57cfccd10a2131fd1e849fce4e5cfd1d
Bug 1244889 - Amend wasm opcodes for unsigned I32x4 operations. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a9da056716a9ee94803d39249cb0673599d8969
Bug 1244889 - Handle all SIMD types in js::SimdTypeToName. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b882758500e400460ca9a799a71e074be9f457
Bug 1244889 - Add support for Uint32x4 as an asm.js type. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c87ee12ce0eaf255ac35aa0ea8c90b4fc10717f
Bug 1244889 - Fix Float32x4toUint32x4 for asm.js. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/94dbdf1ea450accedc55b8d37f63c23c0a277ebd
Bug 1244889 - Disallow unsigned SIMD types for global variables. r=luke

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e82a5bd3381e
https://hg.mozilla.org/mozilla-central/rev/5498464cedbf
https://hg.mozilla.org/mozilla-central/rev/ee4b5764a109
https://hg.mozilla.org/mozilla-central/rev/66b1403ab173
https://hg.mozilla.org/mozilla-central/rev/3577a7648ce7
https://hg.mozilla.org/mozilla-central/rev/70b848b3abc6
https://hg.mozilla.org/mozilla-central/rev/746c90d31e35
https://hg.mozilla.org/mozilla-central/rev/16203ccd8f80
https://hg.mozilla.org/mozilla-central/rev/ecdf89af57cf
https://hg.mozilla.org/mozilla-central/rev/9a9da056716a
https://hg.mozilla.org/mozilla-central/rev/a4b882758500
https://hg.mozilla.org/mozilla-central/rev/9c87ee12ce0e
https://hg.mozilla.org/mozilla-central/rev/94dbdf1ea450