Skip to content

Adding EVEX encoding support for CV path. #78044

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
Merged
merged 2 commits into from
Nov 17, 2022
Merged

Adding EVEX encoding support for CV path. #78044

merged 2 commits into from
Nov 17, 2022

Conversation

DeepakRajendrakumaran
Copy link
Contributor

@DeepakRajendrakumaran DeepakRajendrakumaran commented Nov 8, 2022 •
edited
Loading

This set of changes adds EVEX encoding logic to remaining emitOutput* paths(not covered in previous 2 commits). With this commit, we should now be able to encode avx512 instructions once those are added.

Notes : I have chosen to use the all encompassing 'emitOutputSimdPrefixIfNeeded' instead of'emitOutputRexOrVexPrefixIfNeeded' even in cases where we know EVEX will not be used for uniformity. Internally it will resolve to 'emitOutputRexOrVexPrefixIfNeeded' etc. This extends to other cases like 'TakesSimdPrefix' of 'TakesVexPrefix' etc.

TO DOs:

  1. We still require kmask support, register support for zmm register and upper(16-31) registers in order to use those features with AVX512. These will be added as separate commits.
  2. We will have to modify 'TakesEvexPrefix' logic. Currently has comments there explaining the nature of logic required there. This will have to be fleshed out once AVX512 instructions are added.
  3. Some EVEX encoding logic like embedded broadcast etc which are not present for non AVX512. This needs to be added later
  4. Move 'W' bit as opcode extension to an instruction table driven approach

@anthonycanino

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 8, 2022
@BruceForstall
Copy link
Contributor

/azp run runtime-coreclr jitstress-isas-avx512

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Wraith2
Wraith2 reviewed Nov 9, 2022
View reviewed changes
src/coreclr/jit/emitxarch.cpp Outdated
// Some of its callers already add VEX prefix and then call this routine.
// Therefore add VEX prefix is not already present.
code = AddVexPrefixIfNeededAndNotPresent(ins, code, size);
// Compute VEX/VEX prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

EVEX/VEX ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! Will fix it in the final non draft version. Thanks for catching that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@BruceForstall
Copy link
Contributor

/azp run runtime-coreclr jitstress-isas-avx512

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BruceForstall
Copy link
Contributor

@DeepakRajendrakumaran The avx512 test pipeline was error-free!

@DeepakRajendrakumaran
Copy link
Contributor Author

@DeepakRajendrakumaran The avx512 test pipeline was error-free!

Yaay!. The other pending PR will handle most of the fails but this is also needed to fix some other paths. I want to add some other stuff to this PR and should have it ready early this week.

@DeepakRajendrakumaran DeepakRajendrakumaran force-pushed the avx512_EvexPaths branch 3 times, most recently from 71c4a3f to 6c0ebd2 Compare November 15, 2022 22:14
@DeepakRajendrakumaran DeepakRajendrakumaran marked this pull request as ready for review November 15, 2022 23:00
@DeepakRajendrakumaran
Copy link
Contributor Author

@BruceForstall @tannergooding This is ready for review now. This should be the last change required in this series(EVEX in output path)

@BruceForstall
Copy link
Contributor

/azp run runtime-coreclr jitstress-isas-avx512

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BruceForstall
Copy link
Contributor

@BruceForstall @tannergooding This is ready for review now. This should be the last change required in this series(EVEX in output path)

You should edit the top comment so it doesn't read "Draft PR to run tests" anymore, but is more descriptive of the changes.

@DeepakRajendrakumaran
Copy link
Contributor Author

@BruceForstall @tannergooding This is ready for review now. This should be the last change required in this series(EVEX in output path)

You should edit the top comment so it doesn't read "Draft PR to run tests" anymore, but is more descriptive of the changes.

Have updated it with relevant info.

@build-analysis build-analysis bot mentioned this pull request Nov 16, 2022
Closed
@BruceForstall
Copy link
Contributor

No asm diffs but a throughput regression on x64 of about 0.03%.

@BruceForstall BruceForstall merged commit 98fcb3d into dotnet:main Nov 17, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2022
@BruceForstall BruceForstall added the avx512 Related to the AVX-512 architecture label Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@Wraith2 Wraith2 Wraith2 left review comments

@BruceForstall BruceForstall BruceForstall approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI avx512 Related to the AVX-512 architecture
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DeepakRajendrakumaran @BruceForstall @Wraith2