Skip to content

Conversation

@kunalspathak
Copy link
Contributor

We were hitting an assert in these tests because we were not handling the wrapping of these intrinsics inside `ConditionalSelect.

  • Make sure that ConvertTo* APIs get contained inside ConditionalSelect. To do that we now store the auxiliary type as baseType of return vector and if that matches with the wrapped mask operation, then we contain the intrinsics.
  • Fix the codegen depending on the variant of the operation (see below)

ConvertToInt32 implements https://docsmirror.github.io/A64/2023-06/fcvtzu_z_p_z.html for following variants:

  • Single-precision to 32-bit - FCVTZS <Zd>.S, <Pg>/M, <Zn>.S
  • Double-precision to 32-bit - FCVTZS <Zd>.S, <Pg>/M, <Zn>.D

ConvertToUInt32 implements https://docsmirror.github.io/A64/2023-06/fcvtzs_z_p_z.html for following variants:

  • Single-precision to 32-bit - FCVTZU <Zd>.S, <Pg>/M, <Zn>.S
  • Double-precision to 32-bit - FCVTZU <Zd>.S, <Pg>/M, <Zn>.D

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 24, 2024
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@kunalspathak
Copy link
Contributor Author

@dotnet/arm64-contrib @amanasifkhalid PTAL

@ebepho - For ConvertTo*Int64, you need to follow along this path.

@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Jun 24, 2024
Copy link
Contributor

@amanasifkhalid amanasifkhalid 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 for fixing this!


// For now, make sure that we get here only for intrinsics that we are
// sure about to rely on auxilary type's size.
assert((embOp->GetHWIntrinsicId() == NI_Sve_ConvertToInt32) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you intend for this assert to go away once @ebepho implements ConvertToInt64, or should we keep checking for intrinsics with different base/return types? If the latter, I wonder if we ought to add a flag for these types of intrinsics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, we will update the assert, but may be later, we might consider adding a flag. For now, I want to see if there are more intrinsic categories that falls under this.

kunalspathak and others added 2 commits June 23, 2024 22:26
Co-authored-by: Aman Khalid <amankhalid@microsoft.com>
Co-authored-by: Aman Khalid <amankhalid@microsoft.com>
@kunalspathak kunalspathak merged commit b112eac into dotnet:main Jun 24, 2024
@kunalspathak kunalspathak deleted the sve-convertto branch June 24, 2024 12:36
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI arm-sve Work related to arm64 SVE/SVE2 support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants