Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

NaN behavior for SIMD.{U}Int32x4.FromFloat32x4() conversion #290

Closed
nmostafa opened this issue Oct 16, 2015 · 5 comments
Closed

NaN behavior for SIMD.{U}Int32x4.FromFloat32x4() conversion #290

nmostafa opened this issue Oct 16, 2015 · 5 comments

Comments

@nmostafa
Copy link
Contributor

Currently the polyfill and tests consider NaN in-range. This is because the range check is implemented with existing ECMASript semantics (NaN < 0 || NaN > MAXINT == false). Then the conversion is done with To{U}Int32 semantics where NaNs map to 0.

This conversion doesn't match IA convert instruction where NaNs -> 0x80000000. The SSE implementation of this conversion is already long and adding this check would require 3 more instructions.

Is this the intended behavior from the spec ? Does it make sense for a developer to have NaN silently convert to 0 ? Or do we need to modify the range check semantics ?

Since we already have the range check in the spec, I am in favor of tailoring it towards performance benefit.

Any thoughts on this ?

@billbudge
Copy link
Collaborator

I think the spec was changed to use the builtin ToLength, so the polyfill and tests should be updated to reflect this.

#283

@nmostafa
Copy link
Contributor Author

You mean use ToLength to cast to Int32 ? I don't see that change in the spec. We still use SIMDCreate which calls cast (ToInt32/ToUint32). And even with ToLength, NaN still maps to +0.

We could modify the range check to our advantage here, since the language is already vague in the spec.

For element in list,
    If SIMD is an integer type and TIMD is a floating point type, and element is greater than the maximum value or less than the minimum value of SIMD, throw a RangeError exception.

@littledan
Copy link
Member

NaN out of range makes sense to me. @nmostafa , would you be interested in writing a pull request against the spec text to clarify this? I think we're using ToLength, see http://tc39.github.io/ecmascript_simd/#simd-to-lane .

@nmostafa
Copy link
Contributor Author

Sure. I will do that.

@littledan
Copy link
Member

Pull request has been merged. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants