-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add VectorTableList and TableVectorExtension intrinsics #35600
Add VectorTableList and TableVectorExtension intrinsics #35600
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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. |
Codegen looks like this:
|
c842607
to
ce2c636
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (with minor changes)
#define PERFSCORE_THROUGHPUT_5X 0.20f // Pentuple issue | ||
#define PERFSCORE_THROUGHPUT_4X 0.25f // Quad issue | ||
#define PERFSCORE_THROUGHPUT_3X (1.0f / 3.0f) // Three issue | ||
#define PERFSCORE_THROUGHPUT_2X 0.5f // Dual issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to Tamar's change but I am wondering once again whether we want to move this into some kind of table instead of defining the numbers in code? @BruceForstall @briansull
I spend non-trivial amount of time adding these numbers while working on #35379 and #33461 and I want either simplify the process for PerfScore or make it optional (i.e. remove the assert for unhandled cases and track the work for adding PerfScore numbers as a separate work item) since technically it's not require for the product.
I also see the benefits of moving this to table since if we want we can extend/define a new "table" for another microarchitecture - while now the numbers are "hardcoded" all over the place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that making this table-driven would probably be simpler in the long run. And I'm ok with making it optional, to avoid blocking your forward progress on adding perf scores, although if we do that we should add GitHub issues to track known work (like perfscores needed for instructions X, Y, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good Bruce, I will open such issue to track this when I have another PR adding new instructions
ce2c636
to
3e628b1
Compare
3e628b1
to
f86bd86
Compare
Just realized that since this PR adds new instruction to encoder CodeGen::genArm64EmitterUnitTests in codegenarm64.cpp also needs to be updated. I am merging this anyway to avoid conflicts with #35612 @TamarChristinaArm can you please follow up on this later and add the unit tests? Not sure why runtime (Installer Build and Test coreclr Windows_NT_x86 Debug) shows as In progress - it's finished here, ignoring... |
@echesakovMSFT oops yes I completely forgot about the emitter tests. I'll send a PR today. Thanks! |
Hi All,
This add
VectorTableList
andVectorTableExtension
intrinsicsalong with codegen and jit support for all
TBX
andTBL
variants.Implements the API approved part of #1277.
/CC @echesakovMSFT @tannergooding @CarolEidt
Thanks,
Tamar