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

JIT: Fixed containment of STOREIND of HW intrinsics ConvertTo* #92396

Merged
merged 9 commits into from
Sep 23, 2023

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Sep 21, 2023

Resolves #92349 regarding the truncating CAST

Description

As an example, the HW Intrinsic SIMD scalar ConvertToInt32 will emit a vmovd instruction. This instruction can store a scalar value in memory or in a register, but only for 32 and 64 bits.

So, when we have something like this:

// pValue is a 'ushort*'
*pValue = (ushort)(uint)Sse2.ConvertToInt32(newMinZ16);

Acceptance Criteria

  • Add test case

Note

We need to backport this to .NET 8.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 21, 2023
@ghost ghost assigned TIHan Sep 21, 2023
@TIHan
Copy link
Contributor Author

TIHan commented Sep 21, 2023

cc @tannergooding

@ghost
Copy link

ghost commented Sep 21, 2023

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

Issue Details

Resolves #92349 regarding the truncating CAST

Description

As an example, the HW Intrinsic SIMD scalar ConvertToInt32 will emit a vmovd instruction. This instruction can store a scalar value in memory or in a register, but only for 32 and 64 bits.

So, when we have something like this:

// pValue is a 'ushort*'
*pValue = (ushort)(uint)Sse2.ConvertToInt32(newMinZ16);

We need to keep the cast operation for int -> short, otherwise we will get an incorrect result which is what #92349 was telling us.

I tried to see if there was something we could do in codegen, but it is a lot easier to handle in morph where we get rid of these CASTs. The simplest thing to do is check to see if we have a SIMD scalar operation, and if we do, do not remove the CAST.

Acceptance Criteria

  • Add test case
Author: TIHan
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@TIHan TIHan marked this pull request as draft September 21, 2023 02:36
@jakobbotsch
Copy link
Member

jakobbotsch commented Sep 21, 2023

We need to keep the cast operation for int -> short, otherwise we will get an incorrect result which is what #92349 was telling us.

I tried to see if there was something we could do in codegen, but it is a lot easier to handle in morph where we get rid of these CASTs. The simplest thing to do is check to see if we have a SIMD scalar operation and we are storing the result for a small type, then do not remove the CAST.

We should fix this in the backend. STOREIND<ushort> should never be storing more than two bytes. With this PR the bug still exists, it's just harder to expose (but can definitely be done, e.g. you can write IL manually that doesn't create these casts in the first place).

It looks like a bug in ContainCheckStoreIndir. This line:

isContainable = varTypeIsIntegral(simdBaseType);

should be

isContainable = varTypeIsIntegral(simdBaseType) && (genTypeSize(src) == genTypeSize(node));

if vmovd has no 8/16 bit forms.

@TIHan
Copy link
Contributor Author

TIHan commented Sep 21, 2023

We should fix this in the backend

Your fix works and is a lot simpler than what I had. Thank you.

@TIHan TIHan changed the title JIT: Do not remove CAST on SIMD scalar operations for stores JIT: Fixed containment of STOREIND of HW intrinsics ConvertTo* Sep 21, 2023
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

Add a test?

@TIHan TIHan marked this pull request as ready for review September 21, 2023 20:03
@TIHan
Copy link
Contributor Author

TIHan commented Sep 21, 2023

@dotnet/jit-contrib @jakobbotsch @tannergooding This is ready, pending CI.

@TIHan TIHan merged commit 8c09075 into dotnet:main Sep 23, 2023
131 checks passed
@TIHan
Copy link
Contributor Author

TIHan commented Sep 23, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6280549647

@ghost ghost locked as resolved and limited conversation to collaborators Oct 23, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect value after truncating cast
3 participants