Improve likely class handling for pgo#51284
Conversation
ghost
added
the
|
@dotnet/crossgen-contrib @dotnet/jit-contrib |
| } | ||
| else | ||
| { | ||
| int currentObjectIndex = 0x1000000; |
There was a problem hiding this comment.
should this be defined as a constant?
There was a problem hiding this comment.
Eh, the value is only used one place, and really just needs to be a large non-zero number. I'll leave a comment instead.
|
LGTM from cg2 side. |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
I'm ok with this approach of calling back into the jit. cc @BruceForstall in case he has any thoughts.
Seems like the histogram processing should be in some new file.
Do you know if this works properly for cross-bitness jitting? I am not sure how some of these types vary with target -- seems like INT_PTR might be tied to the host?
| uint8_t ** pInstrumentationData // OUT: `*pInstrumentationData` is set to the address of the instrumentation data. | ||
| ) = 0; | ||
|
|
||
| // Get the likely implementing class for a virtual call or interface call made by ftnHnd |
There was a problem hiding this comment.
Changing the Jit Interface requires an update to JITEEVersionIdentifier located in jiteeversionguid.h
There was a problem hiding this comment.
Oops, I thought I had done that.
INT_PTR is tied to the host, but that's ok for crossbitness jitting as the INT_PTR is simply the contract for JIT/VM communication for class types. For storage into the PE file there is a different format which is token based. (and sadly somewhat verbose, which is why the compressed representation here is quite important.) |
| if (classType != IntPtr.Zero) | ||
| { | ||
| TypeDesc type = (TypeDesc)handleToObject[classType]; | ||
| if (compilationModuleGroup.VersionsWithType(type)) |
There was a problem hiding this comment.
| if (compilationModuleGroup.VersionsWithType(type)) | |
| #if READYTORUN | |
| if (compilationModuleGroup.VersionsWithType(type)) | |
| #endif |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
We will likely want something like this for merging as well...
| XX Likely Class Processing XX | ||
| XX XX | ||
| XX Parses Pgo data to find the most likely class in use at a given XX | ||
| XX in a method. Used by both the JIT, and by crossgen XX |
There was a problem hiding this comment.
| XX in a method. Used by both the JIT, and by crossgen XX | |
| XX offset in a method. Used by both the JIT, and by crossgen. XX |
maybe?
There was a problem hiding this comment.
Yes. I'm not sure where the IL offset bit of my comment went... I certainly remember writing it. Thanks for noting this.
|
This should mostly address #45286 |
|
@davidwrighton I'm late to the party here...
Why can't it be on the JIT-EE interface? |
8 participants
Areas where I would particularly like to see comments