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

Ensure Vector.Sum uses SSE3, rather than SSSE3, for floating-point #54123

Merged
merged 4 commits into from
Jun 14, 2021

Conversation

tannergooding
Copy link
Member

This resolves #54119 and was introduced in #53527

CC. @zlatanov, @echesakovMSFT

@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 Jun 13, 2021
@tannergooding
Copy link
Member Author

tannergooding commented Jun 13, 2021

This wasn't caught by the ISA tests that were run as those only run for the runtime side tests and only library side tests were added.

I've added a runtime side test, mirroring the other runtime tests for Vector, to help ensure this gets the per-ISA validation.

@tannergooding
Copy link
Member Author

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@zlatanov
Copy link
Contributor

Thanks @tannergooding. Looking at this now I can see that SSSE3 HorizontalAdd is not supported for floats and doubles, but I must have missed it when using it.

I actually tried to see if somehow the macros that are generating the NI_ enums could also have comments generated with them, which would include information for which native types the instruction is supported, but I failed.

For example I think it would be invaluable to have something like:

/// <summary>Supported for ...</summary>
NI_SSSE3_HorizontalAdd

@tannergooding
Copy link
Member Author

For example I think it would be invaluable to have something like:

That information is all in the metadata table here (either an instruction or INS_invalid, there are a few cases where invalid can still generate something, but those are rare and typically "helper" intrinsics): https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/hwintrinsiclistxarch.h#L348

The expansion of a macro isn't normally visible without explicitly running an explicit compilation step and saving the pre-processor output, so I don't think having a comment would be beneficial.

@tannergooding
Copy link
Member Author

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tannergooding
Copy link
Member Author

tannergooding commented Jun 14, 2021

ARM64 failures are unrelated (#54125).

@tannergooding tannergooding merged commit d95bfea into dotnet:main Jun 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2021
@tannergooding tannergooding deleted the fix-54119 branch November 11, 2022 15:30
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.

Failed System.Numerics.Tests.GenericVectorTests tests on CI
3 participants