Skip to content
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

ARM64-SVE: Add ConvertToSingle, ConvertToDouble; fix CovertTo* tests #104478

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

amanasifkhalid
Copy link
Member

Part of #99957, and follow-up to #104259. I noticed the ConvertTo* tests weren't correctly testing cases where we convert from larger types to smaller types (for example, see the odd/even lane behavior for SCVTF). I've updated the test helper methods to check for these cases, and the stress tests are passing for existing ConvertTo* APIs (Int32, UInt32, Int64, UInt64).

@dotnet/arm64-contrib PTAL, thanks!

@amanasifkhalid amanasifkhalid added the arm-sve Work related to arm64 SVE/SVE2 support label Jul 5, 2024
@amanasifkhalid
Copy link
Member Author

As of writing, all the stress tests are passing for ConvertToDouble. The only scenarios failing for ConvertToSingle are some of the ConditionalSelect scenarios, when converting a long or ulong to float. Here's an example failing test (I tweaked the error output, such that helper is the output of Helpers.ConvertToSingle(ulong[]), and ours is the maskedVectorResult we built using the helper output, and the mask; result is the actual API's result):

Sve.ConvertToSingle<Single>(Vector<UInt64>): ConditionalSelectScenario_TrueValue failed:
    mask: (1, 0, 0, 0)
    left: (12085043987715027774, 17070722318609634090)
  helper: (1.2085043E+19, 0, 1.7070723E+19, 0)
 falseOp: (1.2085043E+19, 1.7070723E+19, 0, 0)
  result: (1.2085043E+19, 0, 0, 0)
    ours: (1.2085043E+19, 1.7070723E+19, 0, 0)

I wonder if the actual API is reading the mask vector in 64-bit chunks instead of 32-bit ones, such that it sees (true, false) instead of (true, false, false, false). That might explain why the second element in result is from helper instead of falseOp? Or maybe I'm misinterpreting the usage of mask here? Here is a snippet of the disasm:

            ldp     q8, q9, [x19, #0x10]
            ptrue   p0.s
            cmpne   p0.s, p0/z, z8.s, #0
            movi    v16.4s, #0
            movprfx z10, z16
            ucvtf   z10.s, p0/m, z9.d

@kunalspathak
Copy link
Member

and follow-up to #104259.

Can we close #104259?

@amanasifkhalid
Copy link
Member Author

Can we close #104259?

I think that's fine.

@kunalspathak
Copy link
Member

The only scenarios failing for ConvertToSingle are some of the ConditionalSelect scenarios

I ran into similar issue for Compare and it was because we need to use BitConverter.SingleToInt32Bits / BitConverter.Int32BitsToSingle and vice versa for double.

@amanasifkhalid
Copy link
Member Author

I ran into similar issue for Compare and it was because we need to use BitConverter.SingleToInt32Bits / BitConverter.Int32BitsToSingle and vice versa for double.

I see, let me take a look at that locally...

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

@amanasifkhalid
Copy link
Member Author

@kunalspathak it looks like the casting logic is fine, but when compiling the CndSel scenarios for ConvertToSingle, maskSize and operSize here are both 8 bytes, hence why the mask seems to be applied in 8-byte chunks for the failing scenarios. I noticed the same behavior for other Convert APIs that narrow their inputs, like ConvertToInt32(Vector<double>), though those tests haven't failed yet... I'm guessing we're using the SIMD base type of the intrinsic instead of its auxiliary type somewhere, when creating the mask nodes?

@kunalspathak
Copy link
Member

@amanasifkhalid - are you unblocked on this or still investigating?

@amanasifkhalid
Copy link
Member Author

amanasifkhalid commented Jul 9, 2024

@kunalspathak I figured out where we are getting the mask size wrong: When lowering intrinsics with embedded masks here, we need to use the auxiliary SIMD type for the ConvertTo APIs instead of the base type (and ditto for simdSize and simdType?). I adjusted the lowering logic to use the auxiliary JIT type, though I'm still seeing sporadic failures for some of the ConvertToSingle ConditionalSelect scenarios, when converting a 64-bit integer to a 32-bit float. For example:

Sve.ConvertToSingle<Single>(Vector<Int64>): ConditionalSelectScenario_TrueValue failed:
    mask: (1, 0, 0, 0)
    left: (5244210276444988272, 5594561089266975675)
  helper: (5.2442103E+18, 0, 5.594561E+18, 0)
 falseOp: (5.2442103E+18, 5.594561E+18, 0, 0)
  result: (5.2442103E+18, 0, 0, 0)
    ours: (5.2442103E+18, 5.594561E+18, 0, 0)

I wonder if I'm misinterpreting how the Convert APIs should work with ConditionalSelect, and thus testing for the wrong thing? The docs don't specify the predicate register's functionality, though based on the signature of Sve.ConditionalSelect, I'd expect the mask and right operands to match the type of ConvertToSingle's return type. So I'm assuming I am testing for the right thing, and just need additional handling in the JIT to get the ConditionalSelect operands right...

@kunalspathak
Copy link
Member

I'd expect the mask and right operands to match the type of ConvertToSingle's return type

that's right

@amanasifkhalid
Copy link
Member Author

@kunalspathak I updated the test template to skip the CndSel scenarios when the size of the input does not match the size of the return type. Stress tests are now passing.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@amanasifkhalid
Copy link
Member Author

/ba-g unrelated NativeAOT infra failure

@amanasifkhalid amanasifkhalid merged commit 20f11b0 into dotnet:main Jul 16, 2024
163 of 167 checks passed
@amanasifkhalid amanasifkhalid deleted the sve-convert branch July 16, 2024 13:26
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants