Skip to content

Fix the isEmbeddedMaskingCompatible checks #117607

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

Merged
merged 4 commits into from
Jul 16, 2025

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Jul 14, 2025

This resolves #117605

The mask base size corresponds to the number of elements processed in a V128. This can be Count (normal vector), 1 (scalar vector), or Count / n (typically used for conversions; like float->double where the counts mismatch between input/output). While the input base size corresponds to the size of the element type

This means the inputSize for V128<ulong> is 8 while the maskBaseSize is typically 2 (because 16 / 8 == 2) and the embedded mask checks need to checking for 2 for the conversion to be safe.

@Copilot Copilot AI review requested due to automatic review settings July 14, 2025 13:37
@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 14, 2025
@tannergooding
Copy link
Member Author

/azp run Fuzzlyn

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

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

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adjusts the embedded mask compatibility logic to reflect that mask base sizes should be 2 or 4 (instead of 4 or 8) and adds a new JIT regression test for the updated behavior.

  • Renamed and updated the boolean flag and assertions in gentree.cpp to use mask sizes 2 and 4.
  • Changed the compatibility check to use tgtMaskBaseSize == 2.
  • Introduced a new JIT regression test project and test file for Runtime_117605.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/coreclr/jit/gentree.cpp Updated mask base size checks from 4/8 to 2/4
src/tests/JIT/Regression/JitBlue/Runtime_117605/Runtime_117605.csproj Added project file for the new regression test
src/tests/JIT/Regression/JitBlue/Runtime_117605/Runtime_117605.cs Added the generated JIT regression test implementation
Comments suppressed due to low confidence (1)

@tannergooding tannergooding requested a review from EgorBo July 14, 2025 16:34
@tannergooding
Copy link
Member Author

CC. @dotnet/jit-contrib for review. This fixes an assert that was showing up in Fuzzlyn

@tannergooding
Copy link
Member Author

Logged #117612 for the unrelated Arm64 fuzzlyn failure

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

LGTM with nits in the test

@tannergooding
Copy link
Member Author

/ba-g unrelated android timeout and auth failure which didn't show in previous run.

@tannergooding tannergooding merged commit 9897a3b into dotnet:main Jul 16, 2025
114 of 117 checks passed
@tannergooding tannergooding deleted the fix-117605 branch July 16, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

[Fuzzlyn] Assert in isEmbeddedMaskingCompatible
2 participants