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.

Sign up for GitHub

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

Jump to bottom

AVX10.1 API introduction in JIT #101938

Merged
merged 22 commits into from
Jun 9, 2024
Merged

AVX10.1 API introduction in JIT #101938

merged 22 commits into from
Jun 9, 2024
+11,280 −555

Conversation

khushal1996
Copy link
Contributor

@khushal1996 khushal1996 commented May 6, 2024 •
edited

This PR tracks addition of AVX10.1 APIs in libraries along with relevant template tests and intrinsics in JIT. It also captures usage of the new intrinsics in JIT during lowering.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels May 6, 2024
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 6, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@khushal1996 khushal1996 force-pushed the kcm-avx10v1-api-enablement branch 5 times, most recently from 6935056 to e8973b2 Compare May 9, 2024 02:11
This was referenced May 9, 2024
Open
Closed
@khushal1996 khushal1996 force-pushed the kcm-avx10v1-api-enablement branch 2 times, most recently from b84e8fd to b2c417f Compare May 10, 2024 06:56
This was referenced May 10, 2024
Open
Open
Open
@build-analysis build-analysis bot mentioned this pull request May 10, 2024
Open
@khushal1996 khushal1996 force-pushed the kcm-avx10v1-api-enablement branch 2 times, most recently from cefb060 to 538e0bf Compare May 11, 2024 01:53
This was referenced May 11, 2024
Open
Open
@khushal1996 khushal1996 force-pushed the kcm-avx10v1-api-enablement branch 4 times, most recently from e42f02b to af22395 Compare May 11, 2024 03:27
@build-analysis build-analysis bot mentioned this pull request May 13, 2024
Open
@build-analysis build-analysis bot mentioned this pull request May 14, 2024
Open
@tannergooding
Copy link
Member

Changes mostly look good to me. There's some stray changes that are conflicting with PRs that went in over the past couple days that need to be resolved and some potential cleanup, but I think this is just about ready to merge

This was referenced Jun 4, 2024
Open
Open
Open
Open
@khushal1996 khushal1996 mentioned this pull request Jun 4, 2024
Draft
@tannergooding
Copy link
Member

CC. @dotnet/jit-contrib PR from Intel is ready for review and needs secondary sign-off.

I've given this a decently thorough review already and plan on giving it one more pass before sign-off, but it could definitely do with another pair of eyes.

@JulieLeeMSFT
Copy link
Member

@EgorBo please review the PR for AVX10.1.

@JulieLeeMSFT JulieLeeMSFT requested a review from EgorBo June 5, 2024 23:28
EgorBo
EgorBo reviewed Jun 6, 2024
View reviewed changes
src/coreclr/jit/importercalls.cpp Outdated

retNode =
gtNewSimdHWIntrinsicNode(TYP_SIMD16, op1, op2, op3, NI_FMA_MultiplyAddScalar, callJitType, 16);
GenTree* op3 = gtNewSimdCreateScalarUnsafeNode(TYP_SIMD16, impPopStack().val, callJitType, 16);
Copy link
Member

Choose a reason for hiding this comment

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

why do we remove impImplicitR4orR8Cast from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like a merge error when rebasing. cleared it up to what it was.

This was referenced Jun 6, 2024
Open
Open
@EgorBo
Copy link
Member

EgorBo commented Jun 6, 2024

@MihuBot

EgorBo
EgorBo approved these changes Jun 6, 2024
View reviewed changes
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.

I think the RyuJIT side is ok with the Tanner's comments. Do we want to run some ISA outerloops?

@MihuBot MihuBot mentioned this pull request Jun 6, 2024
Open
@MihaZupan MihaZupan mentioned this pull request Jun 6, 2024
Open
@EgorBo
Copy link
Member

EgorBo commented Jun 6, 2024 •
edited

Any idea why this PR causes big diffs on an Intel cpu (must be some Skylake/Cascade Lake cpu on Azure): MihuBot/runtime-utils#403 ? I presume it's supposed to be no-op

@DeepakRajendrakumaran
Copy link
Contributor

Any idea why this PR causes big diffs on an Intel cpu (must be some Skylake/Cascade Lake cpu on Azure): MihuBot/runtime-utils#403 ? I presume it's supposed to be no-op

Something is off here. We are not inlining the Vector512 method in some of these and resorting to software fallback

@DeepakRajendrakumaran
Copy link
Contributor

Any idea why this PR causes big diffs on an Intel cpu (must be some Skylake/Cascade Lake cpu on Azure): MihuBot/runtime-utils#403 ? I presume it's supposed to be no-op

Something is off here. We are not inlining the Vector512 method in some of these and resorting to software fallback

There is a bug in there re ordering of flags being set. It got introduced when we added _EVEX flag. Will add a fix today

@khushal1996
Reorder declaration of InstructionSet_EVEX to proper position. Run fo…
e21bcff 
…rmatting adn resolve errors introduced when merging with main
This was referenced Jun 7, 2024
Open
Open
Open
@khushal1996
Copy link
Contributor Author

khushal1996 commented Jun 7, 2024 •
edited

Any idea why this PR causes big diffs on an Intel cpu (must be some Skylake/Cascade Lake cpu on Azure): MihuBot/runtime-utils#403 ? I presume it's supposed to be no-op

@EgorBo I have pushed a fix and here are the diffs

Ice lake
image

Cascade Lake
image

@tannergooding tannergooding merged commit b5948bf into dotnet:main Jun 9, 2024
169 of 172 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DeepakRajendrakumaran DeepakRajendrakumaran DeepakRajendrakumaran left review comments

@EgorBo EgorBo EgorBo approved these changes

@tannergooding tannergooding tannergooding approved these changes

@fanyang-mono fanyang-mono Awaiting requested review from fanyang-mono fanyang-mono is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

@jeffhandley jeffhandley Awaiting requested review from jeffhandley

@maraf maraf Awaiting requested review from maraf

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@ilonatommy ilonatommy Awaiting requested review from ilonatommy

@lewing lewing Awaiting requested review from lewing

@pavelsavara pavelsavara Awaiting requested review from pavelsavara

@lambdageek lambdageek Awaiting requested review from lambdageek

@steveisok steveisok Awaiting requested review from steveisok

@BrzVlad BrzVlad Awaiting requested review from BrzVlad

@kotlarmilos kotlarmilos Awaiting requested review from kotlarmilos

@thaystg thaystg Awaiting requested review from thaystg

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@khushal1996 @fanyang-mono @tannergooding @JulieLeeMSFT @EgorBo @DeepakRajendrakumaran