Uh oh!
There was an error while loading. Please reload this page.
Devirtualization with multiple guesses: JIT - #86809
Conversation
ghost
commented
May 26, 2023
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis PR enables GDV with multiple candidates for JIT. (did the initial infra and enabled that for NativeAOT) publicinterfaceIValue{intGetValue();}publicclassMyClass1:IValue{publicintGetValue()=>10;}publicclassMyClass2:IValue{publicintGetValue()=>50;}publicclassMyClass3:IValue{publicintGetValue()=>100;}publicclassProgram{publicstaticvoidMain(string[]args){for(inti=0;i<200;i++){Test(newMyClass1());Test(newMyClass1());Test(newMyClass2());Test(newMyClass3());Thread.Sleep(16);}}[MethodImpl(MethodImplOptions.NoInlining)]staticintTest(IValuevalue)=>value.GetValue();}JIT's codegen for ; Assembly listing for method Program:Test(IValue):intsubrsp,40movr11, qword ptr [rcx]movrax,0x7FFBC9FE9C28 ;; is it MyClass1?cmpr11,raxjne SHORT G_M7592_IG04moveax,10jmp SHORT G_M7592_IG09G_M7592_IG04: movrax,0x7FFBC9FE9DF0 ;; is it MyClass2?cmpr11,raxjne SHORT G_M7592_IG06moveax,50jmp SHORT G_M7592_IG09G_M7592_IG06: movrax,0x7FFBC9FE9FB8 ;; is it MyClass3?cmpr11,raxjne SHORT G_M7592_IG08moveax,100jmp SHORT G_M7592_IG09G_M7592_IG08: movr11,0x7FFBC9400310 ;; virtual fallbackcall[r11]IValue:GetValue():int:thisG_M7592_IG09: nopaddrsp,40ret; Total bytes of code 92I will keep the number of candidates to check just
|
EgorBo
commented
May 26, 2023
/azp run runtime-coreclr pgo, runtime-coreclr pgostress, runtime-coreclr libraries-pgo |
|
Azure Pipelines successfully started running 3 pipeline(s). |
EgorBo
commented
May 26, 2023
/azp run runtime-coreclr pgo, runtime-coreclr pgostress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
EgorBo
commented
May 26, 2023
/azp run runtime-coreclr pgo, runtime-coreclr pgostress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
@AndyAyersMS@jakobbotsch PTAL, passes all the tests. I'm going to revert the default number of candidates back to 1 (and enable multiple candidates for pgostress only) - just wanted to make sure it passes CI tests. NOTE: I recommend to review with "Hide whitespaces" option enabled. |
EgorBo
commented
May 27, 2023
/azp run runtime-coreclr pgo, runtime-coreclr pgostress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Stats for the minimal-api (TodosApp) NativeAOT + Exact classes: |
EgorBo
commented
May 30, 2023
PTAL @AndyAyersMS, this also fixed a JitDump-only assert |
| } | ||
| else | ||
| { | ||
| // We're allowed to make more than 2 guesses - pick all types with likelihood >= 10% |
There was a problem hiding this comment.
Note with an 8 entry reservoir the smallest value we'll ever see is 12.5%.
There was a problem hiding this comment.
I assume it's an implementation detail JIT doesn't have to know, right? (and we might change it) Do you want me to change it to 12.5?
There was a problem hiding this comment.
No, just pointing out that currently you may never see a candidate that will fail this test.
Ultimately, we might want to compute the conditional probability. Say there are two candidates with likelihoods of 90 and 9. If the target is not the first candidate, the conditional probability of it being the second candidate is now 90%. (and if there was a third that was say 0.9, if it was not the first two, it is 90% likely to be that third one).
But to do this, we'd have to have more confidence in those low likelihoods.
There was a problem hiding this comment.
Btw, with the current logic, the 10% threshold is likely be ignored for anything more complicated than a simple method because inliner will fine it for too low block weight
| if (IsChainingSupported()) | ||
| { | ||
| call->SetIsGuarded(); |
There was a problem hiding this comment.
@AndyAyersMS it's a theoretical case. IsGuarded is only used for GDV chaining. Imagine we have:
obj.Call1();
obj.Call2();
and Call1() is expanded using 2 classes and chaining is disabled (since it only works with a single candidate). Call2() is expanded with just 1 class - it will try to "chain" with the previous Call1 that doesn't support chaining.
PS: this PR is zero diffs, it doesn't yet enable multiple GDV
There was a problem hiding this comment.
Are you sure? I think IsGuarded is currently just used to annotate the inlining tree and inline XML. It means that a callee was GDV and we now decided to test for it and inline it. So I think you can just call it always here.
The inline tree and XML will get confused when one call site expands to multiple candidates. We should think about how we want to express something like multi-guess GDV for those situations. But that can wait.
There was a problem hiding this comment.
You're right, I've reverted that change as it's not needed
| if (IsChainingSupported()) | ||
| { | ||
| call->SetIsGuarded(); |
| if ((inlineInfo != nullptr) && (inlineInfo->exactContextHnd != nullptr)) | ||
| { | ||
| printf(" (exactContextHnd=0x%p)", dspPtr(inlineInfo->exactContextHnd)); | ||
| } |
There was a problem hiding this comment.
These changes are JitDump only, gtDispTree was not aware that a call might have more than a single candidate.
…sed to produce unexpected diffs
Contributes to #86769
This PR enables GDV with multiple candidates for JIT. (#86551 did the initial infra and enabled that for NativeAOT)
Example:
JIT's codegen for
Test:I will keep the number of candidates to check just
1(current behavior in Main) for JIT untill I implement all work items in #86769