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

[Analyzer] Upgrade from platform specific to cross platform intrinsics #82486

Open
tannergooding opened this issue Feb 22, 2023 · 6 comments
Open
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.Intrinsics code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@tannergooding
Copy link
Member

We've iteratively exposed support for various levels of hardware intrinsics since .NET Core 3.1. In .NET 7, we exposed the new "cross platform" hardware intrinsics which aim to help simplify the writing and maintenance of SIMD code meant to run across a variety of platforms.

As such, we should provide a code fixer that identifies such platform specific calls and suggests the simple transformation to the equivalent cross platform calls.

Category: Maintainability or Portability seem like the best fit. I could also see this being classified as Usage
Severity = suggestion

Example

A simple example is given below. The full list of APIs recognized is of course more expansive but effectively correlates to a platform specific ISA such as Sse.Add, Sse2.Add, Avx.Add, Avx2.Add, AdvSimd.Add, or PackedSimd.Add correlating to a singular cross platform API, such as operator +:

- return Sse.Add(Sse.Multiply(left, right), addend);
+ return (left * right) + addend;
@tannergooding tannergooding added area-System.Runtime.Intrinsics code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 22, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 22, 2023
@ghost
Copy link

ghost commented Feb 22, 2023

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

Issue Details

We've iteratively exposed support for various levels of hardware intrinsics since .NET Core 3.1. In .NET 7, we exposed the new "cross platform" hardware intrinsics which aim to help simplify the writing and maintenance of SIMD code meant to run across a variety of platforms.

As such, we should provide a code fixer that identifies such platform specific calls and suggests the simple transformation to the equivalent cross platform calls.

Category: Maintainability or Portability seem like the best fit. I could also see this being classified as Usage
Severity = suggestion

Example

A simple example is given below. The full list of APIs recognized is of course more expansive but effectively correlates to a platform specific ISA such as Sse.Add, Sse2.Add, Avx.Add, Avx2.Add, AdvSimd.Add, or PackedSimd.Add correlating to a singular cross platform API, such as operator +:

- return Sse.Add(Sse.Multiply(left, right), addend);
+ return (left * right) + addend;
Author: tannergooding
Assignees: -
Labels:

area-System.Runtime.Intrinsics, code-analyzer, code-fixer, api-ready-for-review

Milestone: -

@dakersnar dakersnar removed the untriaged New issue has not been triaged by the area owner label Feb 22, 2023
@dakersnar dakersnar added this to the 8.0.0 milestone Feb 22, 2023
@terrajobst
Copy link
Member

terrajobst commented Feb 23, 2023

Video

Code fixer looks good as proposed.

This would handle most common operators (+, -, /, *, and even Xor).

Either category seems fine, but we have a slight preference for maintainability.

- Sse.Add(Sse.Multiply(left, right), addend)
+ (left * right) + addend

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 23, 2023
@tannergooding
Copy link
Member Author

@buyaa-n, I'd like to start prototyping this. What's required before I start doing that?

Do we need to determine which dotnet/roslyn-analyzers project this will be part of and "allocate" some diagnostic ID(s)?

@buyaa-n
Copy link
Contributor

buyaa-n commented Feb 27, 2023

What's required before I start doing that?

Might want to start with the getting started doc, some info might be a bit old, let me know if you have any questions. Could check other PRs that adding a new analyzer as an example

Do we need to determine which dotnet/roslyn-analyzers project this will be part of and "allocate" some diagnostic ID(s)?

Libraries analyzers goes into Microsoft.NetCore.Analyzers project, for id just pick the next available ID for the category https://github.com/dotnet/roslyn-analyzers/blob/main/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt#L20. Looks last id used for Maintainability category is CA1513, so you can pick CA1514 and update the range in the file accordingly

@TheBlackPlague
Copy link

TheBlackPlague commented Apr 4, 2023

If it's okay, I'd like to pitch in — I'm the developer of StockNemo, a chess engine in C# that heavily makes use of SIMD intrinsics available for its neural network inference.

Please see: https://github.com/TheBlackPlague/StockNemo/blob/f3b52613049c03d8d2a85dfdba417481c6ef795f/Backend/Engine/NNUE/Vectorization/VMethod.cs#L43

Maybe when operations like matrix_o = matrix_x * matrix_w + matrix_b (neural network propagation operation), we can resort to using the fused multiply and add adjacent?

In some cases, this will work better than the regular operations. Let me know what you think.

@tannergooding
Copy link
Member Author

Maybe when operations like matrix_o = matrix_x * matrix_w + matrix_b (neural network propagation operation), we can resort to using the fused multiply and add adjacent?

This isn't a "value-preserving optimization" so its not one we can offer by default. In particular, it will result in different results for "overflow" to +/-infinity and in slightly different results in a few other cases due to rounding once rather than twice.

If it's okay, I'd like to pitch in

I appreciate the offer. We're going to be looking at this closer to the Preview 5/6 timeframe and I'll ping again on the thread around that time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.Intrinsics code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

No branches or pull requests

5 participants