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

Make TYP_SIMD32 be xarch only #82624

Merged
merged 3 commits into from
Feb 24, 2023
Merged

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Feb 24, 2023

Notably this does not also #ifdef the usage of simd32_t. That is a slightly more involved change and will be simpler with the template refactoring that #80960 was adding

@EgorBo
Copy link
Member

EgorBo commented Feb 24, 2023

@tannergooding are +100 LOC of if-defs worth it? My personal experience that if-defs make code harder to read 🙁

@tannergooding
Copy link
Member Author

tannergooding commented Feb 24, 2023

@EgorBo, that's what the PR is for. It's here to measure the TP impact difference.

Based on what we're seeing for the PR adding TYP_SIMD64, it may be decently "significant" (0.05 - 0.15% or so).

@jkoritzinsky
Copy link
Member

Instead of using TARGET_XARCH, would it be possible to introduce a new feature define (FEATURE_SIMD32?) so it's easy to spot which paths are ifdef'd out because they're part of this feature that are currently xarch only as compared to the ifdefs for parts of the code that are always going to be ifdef'd for TARGET_XARCH?

@tannergooding
Copy link
Member Author

tannergooding commented Feb 24, 2023

I don't think it's worth doing so. That's just going to mean we also need FEATURE_SIMD64 and there are no other target architectures out there at the moment which support either features.

Other architectures have instead been introducing "variable width" vectors (e.g. ARM SVE, or what RISC-V has) and the general TYP_SIMD32 support wouldn't apply there either.

If we do end up having to change it in the future, we could look at pulling it out then (and it's relatively simple to do so since it is just finding the few places TYP_SIMD32 is handled.

@tannergooding
Copy link
Member Author

tannergooding commented Feb 24, 2023

Yep, this represents a -0.09 to -0.19% TP improvement for Arm64. It is 100% worth the additional #ifdefs

https://dev.azure.com/dnceng-public/public/_build/results?buildId=183840&view=ms.vss-build-web.run-extensions-tab

CC. @dotnet/jit-contrib

@tannergooding tannergooding marked this pull request as ready for review February 24, 2023 20:02
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM

While it's hard to see new ifdefs added, I appreciate that it improves arm64, and especially allows the incoming TYP_SIMD64 to not affect arm64.

@tannergooding
Copy link
Member Author

Bit less of an improvement with #82616 having been merged (it's -0.01% to -0.06%), but its going to effectively be double that with the TYP_SIMD64 addition in the other PR so still an altogether thing we want to take.

@tannergooding tannergooding merged commit 4aa1571 into dotnet:main Feb 24, 2023
@tannergooding tannergooding deleted the nosimd32-arm64 branch February 24, 2023 23:11
@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants