Get exact class during devirtualization #45526
Conversation
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 dotnet#38477.
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
|
@davidwrighton PTAL cc @dotnet/jit-contrib @Rattenkrieg |
src/coreclr/src/vm/jitinterface.cpp
Outdated
| @@ -9002,33 +9009,36 @@ CORINFO_METHOD_HANDLE CEEInfo::resolveVirtualMethodHelper(CORINFO_METHOD_HANDLE | |||
| Assembly* pCallerAssembly = callerMethod->GetModule()->GetAssembly(); | |||
| bool allowDevirt = | |||
| IsInSameVersionBubble(pCallerAssembly , pDevirtMD->GetModule()->GetAssembly()) | |||
| && IsInSameVersionBubble(pCallerAssembly , pDerivedMT->GetAssembly()); | |||
| && IsInSameVersionBubble(pCallerAssembly, pExactMT->GetAssembly()); | |||
There was a problem hiding this comment.
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.
GetAssembly is not affected by instantiation, but this change loses track of the R2R protection. Instead of using pExactMT should be pObjMT
There was a problem hiding this comment.
Ah, right. We need to check that the full inheritance chain is in the bubble.
|
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 = 40
G_M24375_IG01:
sub rsp, 40
;; bbWeight=1 PerfScore 0.25
G_M24375_IG02:
mov eax, 1
;; bbWeight=1 PerfScore 0.25
G_M24375_IG03:
add rsp, 40
ret
;; 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():intAlso want to look into why the jit thinks it needs to create a frame. |
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. |
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... |
| // | ||
| public CORINFO_METHOD_STRUCT_* devirtualizedMethod; | ||
| public uint _requiresInstMethodTableArg; | ||
| public bool requiresInstMethodTableArg { get { return _requiresInstMethodTableArg != 0; } set { _requiresInstMethodTableArg = value ? (byte)1 : (byte)0; } } |
There was a problem hiding this comment.
There is some entropy wrt passing bool flags between managed and unmanaged code in this file. So far I see uints and bytes used. You probably copied that pattern from CORINFO_CALL_INFO above which uses Windows BOOL on native side. Other structs defined in corinfo.h have fields declared as c++ bool.
There was a problem hiding this comment.
I noticed -- @davidwrighton can you comment on that?
There was a problem hiding this comment.
Yes, bool is byte in managed code and BOOL is uint 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.
| // interface corresponding to pBaseMD. | ||
| if (!pDerivedMT->CanCastToInterface(pBaseMT)) | ||
| if (!pObjMT->CanCastToInterface(pBaseMT)) |
There was a problem hiding this comment.
Perhaps this PR could be a better place to introduce/evaluate this change
There was a problem hiding this comment.
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.
|
One test failure to investigate... |
I guess this one
dotnet/coreclr#16340 mov eax, 0 ; s_result
retmy guess that we should not inline constructor body when class has beforefieldinit. |
|
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... |
|
@AndyAyersMS how did you managed to have jit diffs while having changes on vm side (coreclr.dll)? See very last line of this comment Do you run a patched |
|
I run jit-diffs twice, one for the base and one for the diff, and then jit-analyze to compare the two... |
Likely need to make a matching fix in crossgen2... [edit: actually I think crossgen2 is ok for now because |
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. |
|
@davidwrighton any chance you can look this over? |
| // | ||
| public CORINFO_METHOD_STRUCT_* devirtualizedMethod; | ||
| public uint _requiresInstMethodTableArg; | ||
| public bool requiresInstMethodTableArg { get { return _requiresInstMethodTableArg != 0; } set { _requiresInstMethodTableArg = value ? (byte)1 : (byte)0; } } |
There was a problem hiding this comment.
Yes, bool is byte in managed code and BOOL is uint 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
| @@ -9002,33 +9009,36 @@ CORINFO_METHOD_HANDLE CEEInfo::resolveVirtualMethodHelper(CORINFO_METHOD_HANDLE | |||
| Assembly* pCallerAssembly = callerMethod->GetModule()->GetAssembly(); | |||
| bool allowDevirt = | |||
| IsInSameVersionBubble(pCallerAssembly , pDevirtMD->GetModule()->GetAssembly()) | |||
| && IsInSameVersionBubble(pCallerAssembly , pDerivedMT->GetAssembly()); | |||
| && IsInSameVersionBubble(pCallerAssembly, pExactMT->GetAssembly()); | |||
There was a problem hiding this comment.
GetAssembly is not affected by instantiation, but this change loses track of the R2R protection. Instead of using pExactMT should be pObjMT
ghost
locked as resolved and limited conversation to collaborators
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.