-
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
Adding support for X86Base.Pause() and ArmBase.Yield() #61065
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. |
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThis resolves #53532
|
@@ -16011,6 +16011,13 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins | |||
result.insThroughput = PERFSCORE_THROUGHPUT_2X; | |||
break; | |||
|
|||
case INS_pause: | |||
{ | |||
result.insLatency = PERFSCORE_LATENCY_140C; |
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.
it's 14x lower on pre-Skylakes. It reminds me this issue #8744
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.
Yes, but all of our latencies are currently dependent on and configured for Skylake.
We'd need some more advanced setup if we wanted to track per architecture latencies/throughput.
9e78efa
to
7067fa7
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.
I assume this is no-diff change?
Correct, although it looks like there is some minor issue on Arm64 I might need to fix still. |
@echesakovMSFT, could you take a quick peak at the one additional commit that resolves the ARM64 issue. It's now generating the correct code and working as expected. The issue was that the category wasn't being handled correctly and a few tweaks were needed to handle there being no args, no base type, and no EA_ATTR. |
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.
The last change LGTM.
This resolves #53532