-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implements convert low signed integer to float for x64 simd #2771
Implements convert low signed integer to float for x64 simd #2771
Conversation
b9c597c
to
63ad387
Compare
Also .. this patch is tested against the spec tests for "f64x2.convert_low_i32x4_s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me with the documentation fixes I noted.
Convert signed integer to floating point. | ||
|
||
Each lane in `x` is interpreted as a signed integer and converted to | ||
floating point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add something about rounding like in fcvt_from_sint
and something describing what the low
part of the name means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it's not like the SIMD documentation is much more verbose... perhaps that should get fixed too. I think we need to say something about how the two lowest i32
s in the vector (lanes 0 and 1) are the ones converted to the f64
. From the Intel reference:
CVTDQ2PD (128-bit Legacy SSE version)
DEST[63:0] := Convert_Integer_To_Double_Precision_Floating_Point(SRC[31:0])
DEST[127:64] := Convert_Integer_To_Double_Precision_Floating_Point(SRC[63:32])
DEST[MAXVL-1:128] (unmodified)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a good point. In updating the comments I just used language of half. That is because the "fcvt_low_from_sint" can also support AVX-512 where we could convert the "four" lowest i32s. That instruction doesn't exist of course in the spec, but could be a candidate for a future iteration of the spec or relaxed simd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a +1 to Andrew's thoughts on adding some doc details. Thanks!
63ad387
to
4a48904
Compare
No description provided.