JIT: Support custom ClassLayout instances with GC pointers in them
#112064
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
A number of "Inconsistent profile errors" assertions in the jitstress runs, and also an AV crash on x86. Need to look at those first. |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS The libraries-jitstress failures are being fixed by #112147. Will rerun the CI once that is merged. |
| unsigned hash = key.Size; | ||
| if (key.GCPtrTypes != nullptr) | ||
| { | ||
| hash ^= 0xc4cfbb2a + (hash << 19) + (hash >> 13); |
There was a problem hiding this comment.
Is there any science to these hex values?
There was a problem hiding this comment.
No, I just picked two random values here
There was a problem hiding this comment.
To be clear, the constant used below (0x9e3779b9) is a common one I've seen in hash functions. But here I wanted the hash to always include some information about GCPtrTypes != nullptr or GCPtrTypes == nullptr, so this is folded <random constant> + 0x9e3779b9 for those two cases.
| { | ||
| assert(slot < GetSlotCount()); | ||
| BYTE* ptrs = GetOrCreateGCPtrs(); | ||
| if (ptrs[slot] != TYPE_GC_NONE) |
There was a problem hiding this comment.
Should it be an error to "un gc" a slot?
There was a problem hiding this comment.
I don't have a strong opinion.. I figured it wasn't that hard to support any potential use pattern here. But would be ok with asserting here also.
There was a problem hiding this comment.
Going to merge as is, but let me know if you prefer the opposite and I can switch it in a follow-up.
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Another win-x86 failure... Will have to dig deeper. |
|
Trying to collect some more failures... cannot repro the failure locally after trying for a while in a loop. |
|
/azp run runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I think I tracked down what the problem is: |
|
/azp run runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Failing job has a console log that actually looks like it succeeded. |
* main: (23 commits) add important remarks to NrbfDecoder (dotnet#111286) docs: fix spelling grammar and missing words in clr-code-guide.md (dotnet#112222) Consider type declaration order in MethodImpls (dotnet#111998) Add a feature flag to not use GVM in Linq Select (dotnet#109978) [cDAC] Implement ISOSDacInterface::GetMethodDescPtrFromIp (dotnet#110755) Restructure JSImport/JSExport generators to share more code and utilize more Microsoft.Interop.SourceGeneration shared code (dotnet#107769) Add more detailed explanations to control-flow RegexOpcode values (dotnet#112170) Add repo-specific condition to labeling workflows (dotnet#112169) Fix bad assembly when a nested exported type is marked via link.xml (dotnet#107945) Make `CalculateAssemblyAction` virtual. (dotnet#112154) JIT: Enable reusing profile-aware DFS trees between phases (dotnet#112198) Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory (dotnet#111877) JIT: Support custom `ClassLayout` instances with GC pointers in them (dotnet#112064) Factor positive lookaheads better into find optimizations (dotnet#112107) Add ImmutableCollectionsMarshal.AsMemory (dotnet#112177) [mono] ILStrip write directly to the output filestream (dotnet#112142) Allow the NativeAOT runtime pack to be specified as the ILC runtime package (dotnet#111876) JIT: some reworking for conditional escape analysis (dotnet#112194) Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints (dotnet#112025) [Android] Decouple runtime initialization and entry point execution for Android sample (dotnet#111742) ...
ClassLayoutBuilderthat can be used to build newClassLayoutinstances with arbitrary GC pointersLCL_FLDstress to test some of this new supportSubsumes #111942
Fixes #103362
As future work I also want to add a
StructSegmentsintoClassLayoutso that all layouts carry information about padding/non-padding. Once that is done we should be able to switch object stack allocation to use custom layouts without any regressions. That should allow us to removegetHeapClassSizeandgetTypeForBoxOnStackJIT-EE functions (in addition to follow up work to support promoted arrays of types that may have GC pointers).