-
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
Guarded devirtualization: multiple type checks #86551
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis PR builds a basic infrastructure to enable multiple GDV checks (for all runtimes), but for now it's only enabled for NativeAOT where for daul-morphic interface calls we can devirtualize fallback too, e.g.: public interface IValue
{
int GetValue();
}
public class MyClass1 : IValue
{
public int GetValue() => 10;
}
public class MyClass2 : IValue
{
public int GetValue() => 100;
}
static int Test(IValue val) => val.GetValue(); Old NativeAOT codegen for ; Assembly listing for method Program:Test(IValue):int
sub rsp, 40
lea r10, [(reloc 0x4000000000420060)]
call [r10]IValue:GetValue():int:this ;; interface call, not devirtualized
nop
add rsp, 40
ret
; Total bytes of code 20 New codegen: ; Assembly listing for method Program:Test(IValue):int
lea rax, [(reloc 0x4000000000420fe0)]
mov edx, 100
mov r8d, 10
cmp qword ptr [rcx], rax
mov eax, r8d
cmovne eax, edx
ret
; Total bytes of code 28 (cmove for 10 or 100 based on type check for Should not be difficult to enable for JIT and multiple type checks (but will need some work around chaining)
|
Out of curiosity - what will be the interaction between this and PGO data? If PGO says the likely class is X and whole program analysis says the possibilities are X and Y. Would we trust PGO or the overapproximation from whole program analysis? It's possible this question doesn't make much difference for 2 cases, but if we have PGO say X and whole program analysis say X, Y, Z, U, V, W, maybe we can trust PGO. |
I think we can consult with PGO here yes, but, presumably, it's a rare case for AOT now because Static PGO is quite complicated to setup and the process is not documented (and tested) well. |
/azp list |
This comment was marked as resolved.
This comment was marked as resolved.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
We have PGO data that we ship with the product - one of the things it's trained on is ASP.NET. So this could be relevant for ASP.NET scenarios and doesn't require the complicated setup there because it comes with the compiler. |
I plan to work on this in iterations when I have time so definitely something I'll consider. For now I changed the logic to rely on PGO if it exists, otherwise - go the exact classes list. Also, I completely rewrote the PR so now it has a full-fledged multiple-candidates support (controlled via |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
|
||
elseBlock->setBBProfileWeight(newWeight); | ||
elseBlock->inheritWeightPercentage(currBlock, elseLikelihood); |
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.
For the multi-guess exact case this likelihood should end up at zero, right? We think we know all the possibilities.
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 the logic is shared with non-exact case where fallback may have non-zero likelihood (Dynamic PGO)
I plan to enable "no fallback needed" separately, I am thinking of doing something like "we're importing the last type-check and the call has GTF_M_EXACT_GDV flag --> convert the last pair of check-then to elseBock.
Thanks! I still plan to address some of your comments as part of this PR so will ping you again once I'm done🙂 Meanwhile, I filed #86769 to record the ideas |
/azp run runtime-extra-platforms, runtime-coreclr pgo, runtime-coreclr pgostress |
Azure Pipelines successfully started running 3 pipeline(s). |
@AndyAyersMS I've addressed your feedback - |
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.
Still LGTM.
IIRC 3 is a common value for the number of checks in these sorts of things, once you get past that then (assuming you are testing candidates in decreasing order of likelihood) the cost of that many (possibly mis-predicted) branches starts to overwhelm the benefit.
The MinOpts TP impact of this change seems quite significant, is it expected? Also lots of misses, so hard to know how accurate those diffs are, is it possible to recollect the diffs on the new collection now? |
Agree. Just noticed at the TP numbers. |
Yes, because I call a new JIT-EE API for each virtual call ( The 0.2% MinOpts regression comes from unexpected calls to |
Mitigated via #86949 |
Contributes to #86769 and #86235
This PR builds a basic infrastructure to enable multiple GDV checks (for all runtimes), but for now it's only enabled for NativeAOT:
Demo:
NativeAOT codegen for
Test
(without static pgo):