- Notifications
You must be signed in to change notification settings - Fork5.2k
Get exact class during devirtualization#45526
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
If we devirtualize to a method on a generic class, try and obtain theexact class. Pass this back to the jit to unblock some types of inlines.Also refactor how information is passed during devirtualization inanticipation of follow on work to devirtualize default interface methods.Because there are now multiple inputs and outputs, convey everthingusing a struct.Resolvesdotnet#38477.
AndyAyersMS commentedDec 3, 2020
@davidwrighton PTAL cc @dotnet/jit-contrib@Rattenkrieg |
src/coreclr/src/vm/jitinterface.cpp Outdated
| bool allowDevirt = | ||
| IsInSameVersionBubble(pCallerAssembly , pDevirtMD->GetModule()->GetAssembly()) | ||
| &&IsInSameVersionBubble(pCallerAssembly , pDerivedMT->GetAssembly()); | ||
| &&IsInSameVersionBubble(pCallerAssembly, pExactMT->GetAssembly()); |
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 sure if this change makes sense; for an instantiation which assembly is returned?
Assuming this does make sense, if the exact class would block devirt we might try checking again with the shared class...?
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.
GetAssembly is not affected by instantiation, but this change loses track of the R2R protection. Instead of usingpExactMT should bepObjMT
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.
Ah, right. We need to check that the full inheritance chain is in the bubble.
AndyAyersMS commentedDec 3, 2020
Not many hits here per PMI. Could be something else is still blocking optimization in places. Fixes the case from#38477, we now produce: ; Assembly listing for method Program:Main():int; Emitting BLENDED_CODE for X64 CPU with AVX - Windows; optimized code; rsp based frame; partially interruptible; Final local variable assignments;; V00 OutArgs [V00 ] ( 1, 1 ) lclBlk (32) [rsp+0x00] "OutgoingArgSpace";* V01 tmp1 [V01,T00] ( 0, 0 ) ref -> zero-ref class-hnd exact "NewObj constructor temp";; Lcl frame size = 40G_M24375_IG01:subrsp,40;; bbWeight=1 PerfScore 0.25G_M24375_IG02:moveax,1;; bbWeight=1 PerfScore 0.25G_M24375_IG03:addrsp,40ret;; bbWeight=1 PerfScore 1.25; Total bytes of code 14, prolog size 4, PerfScore 3.15, instruction count 4 (MethodHash=d9b8a0c8) for method Program:Main():int Also want to look into why the jit thinks it needs to create a frame. |
AndyAyersMS commentedDec 3, 2020
This happens because there's a newobj that we dead code, but only after lower, so the jit still thinks the method is going to make a call, and so allocates 40 bytes (8 to align the SP, and 32 for the outgoing args area). I'm surprised we don't eliminate dead code earlier, when we first build liveness. Seems like it might pay for itself in terms of TP. |
AndyAyersMS commentedDec 3, 2020
And this happens because at that point there's a use for the newobj result -- a null check. So it appears to be alive. So why can't local assertion prop get rid of this nullcheck? Looks like it does not know the newobj can't produce a null result. Ironically the nullcheck produces a non-null assertion... |
| // | ||
| publicCORINFO_METHOD_STRUCT_*devirtualizedMethod; | ||
| publicuint_requiresInstMethodTableArg; | ||
| publicboolrequiresInstMethodTableArg{get{return_requiresInstMethodTableArg!=0;}set{_requiresInstMethodTableArg=value?(byte)1:(byte)0;}} |
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.
There is some entropy wrt passing bool flags between managed and unmanaged code in this file. So far I seeuints andbytes used. You probably copied that pattern fromCORINFO_CALL_INFO above which uses WindowsBOOL on native side. Other structs defined incorinfo.h have fields declared as c++bool.
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 noticed --@davidwrighton can you comment on that?
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,bool isbyte in managed code andBOOL isuint in managed code. (And to be pedantic, VARIANT_BOOL is ushort in managed code, but just don't use that type.) Due to the various alignment rules in play the code as written will function correctly on our current set of platforms, but please change the field type to byte.
@Rattenkrieg is completely correct. In a perfect world we'd standardize on bool. In fact, I'd like to see us change our c++ headers so that they are compatible with a standard set of C++ headers instead of requiring the PAL headers, but that's a much larger change that I may attempt over the holidays.
| // We must ensure thatpObjMT actually implements the | ||
| // interface corresponding to pBaseMD. | ||
| if (!pDerivedMT->CanCastToInterface(pBaseMT)) | ||
| if (!pObjMT->CanCastToInterface(pBaseMT)) |
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.
Perhaps this PR could be a better place to introduce/evaluatethis change
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.
We can do that by amending your PR, or as a subsequent PR.
Hopefully what I've done here sets things up so that change is more straightforward.
AndyAyersMS commentedDec 3, 2020
One test failure to investigate... |
Rattenkrieg commentedDec 3, 2020
I guess this one
dotnet/coreclr#16340 moveax,0 ; s_resultret my guess that we should not inline constructor body when class has beforefieldinit. |
AndyAyersMS commentedDec 3, 2020
The bug was that we were failing to find the exact context because for DIMs the class can't be found by searching the inheritance chain. So the exact context was null and the context value 1 (created from null by MAKE_CLASSCONTEXT) has a special meaning which caused the jit to bypass the class init check. Now when we have that case we will just use the class associated with the method... |
Rattenkrieg commentedDec 3, 2020
@AndyAyersMS how did you managed to have jit diffs while having changes on vm side (coreclr.dll)? See very last line ofthis comment Do you run a patched |
AndyAyersMS commentedDec 3, 2020
I run jit-diffs twice, one for the base and one for the diff, and then jit-analyze to compare the two... |
AndyAyersMS commentedDec 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Likely need to make a matching fix in crossgen2... [edit: actually I think crossgen2 is ok for now because |
AndyAyersMS commentedDec 4, 2020
I have prototyped a fix where the jit will no longer create a partial frame (master...AndyAyersMS:NonNullNewAssertion). However, like many assertion prop changes, it also has a downside because it may eliminate other useful assertions. Overall jit-diff impact is small so probably not worth pursuing; per PMI: It might be more surgical to fix this problem by resetting the outgoing arg size accounting before running lower, but again that probably has minimal impact. |
AndyAyersMS commentedDec 4, 2020
@davidwrighton any chance you can look this over? |
davidwrighton left a comment
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 R2R and bool changes are needed, but otherwise I like the change.
| // | ||
| publicCORINFO_METHOD_STRUCT_*devirtualizedMethod; | ||
| publicuint_requiresInstMethodTableArg; | ||
| publicboolrequiresInstMethodTableArg{get{return_requiresInstMethodTableArg!=0;}set{_requiresInstMethodTableArg=value?(byte)1:(byte)0;}} |
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,bool isbyte in managed code andBOOL isuint in managed code. (And to be pedantic, VARIANT_BOOL is ushort in managed code, but just don't use that type.) Due to the various alignment rules in play the code as written will function correctly on our current set of platforms, but please change the field type to byte.
@Rattenkrieg is completely correct. In a perfect world we'd standardize on bool. In fact, I'd like to see us change our c++ headers so that they are compatible with a standard set of C++ headers instead of requiring the PAL headers, but that's a much larger change that I may attempt over the holidays.
src/coreclr/src/vm/jitinterface.cpp Outdated
| bool allowDevirt = | ||
| IsInSameVersionBubble(pCallerAssembly , pDevirtMD->GetModule()->GetAssembly()) | ||
| &&IsInSameVersionBubble(pCallerAssembly , pDerivedMT->GetAssembly()); | ||
| &&IsInSameVersionBubble(pCallerAssembly, pExactMT->GetAssembly()); |
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.
GetAssembly is not affected by instantiation, but this change loses track of the R2R protection. Instead of usingpExactMT should bepObjMT
If we devirtualize to a method on a generic class, try and obtain the
exact class. Pass this back to the jit to unblock some types of inlines.
Also refactor how information is passed during devirtualization in
anticipation of follow on work to devirtualize default interface methods.
Because there are now multiple inputs and outputs, convey everthing
using a struct.
Resolves#38477.