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. |
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).