Fix for Crossgen2 compilation of System.ServiceModel.Syndication #35728
Conversation
|
Is there a way to run asm-diff on this to see what it does to the generated code? |
|
Or at least have some high-level stats like code-sizes before/after. |
|
@jkotas - my apologies, I completely forgot about that. I'll produce a pair of R2RDump outputs for comparison in a bit. |
|
OK, here are the two dumps: https://gist.github.com/trylek/4e5194e71fd824b28b1bfb6c0a89cc26 I used to have a tool called SDVDiff using which I could publish a structured diff of the two dumps but I no longer have it and the codebox link is dead. From what I saw, there are some extra null checks in form of cmp dword ptr [rcx], ecx and some scattered bits of code changes. With the field checks commented out, the size difference for the SSS assembly is 402944 B without the change and 404480 with the change. |
|
This change should be introducing extra null checks. I think there may be something else going on that needs to be understood. |
I originally tried to use the flag CORINFO_FLG_CUSTOMLAYOUT as suggested by JanK but that doesn't work - apparently the code at https://github.com/dotnet/runtime/blob/57a931b11e9ecff42bf75d2e451370af904c0992/src/coreclr/src/jit/lclvars.cpp#L1696 the CUSTOMLAYOUT flag is only honored for HFA fields. Thanks Tomas
I have added a new flag to suppress field promotion for types outside of the compilation version bubble. I have also fixed two other small inconsistencies I noticed in the investigation: 1) I have changed getClassName to use TypeString as the output matches Crossgen1 and makes NGenDump outputs easier to compare. 2) When I was comparing the NGen dumps, I noticed a check in Crossgen1 missing in Crossgen2 - only setting CORINFO_FLG_BEFOREFIELDINIT for types in the same version bubble; so I made it conditional in Crossgen2 too: https://github.com/dotnet/runtime/blob/f9d3e6f2151d090f3fc3eae1965b2339f7a5428e/src/coreclr/src/vm/jitinterface.cpp#L3925 Thanks Tomas
| @@ -1357,6 +1366,14 @@ private uint getClassAttribsInternal(TypeDesc type) | |||
| result |= CorInfoFlag.CORINFO_FLG_ABSTRACT; | |||
| } | |||
|
|
|||
| #if READYTORUN | |||
| if (!_compilation.NodeFactory.CompilationModuleGroup.VersionsWithType(type)) | |||
There was a problem hiding this comment.
Can this be _compilation.CompilationModuleGroup like above?
There was a problem hiding this comment.
Good point, fixed in 3rd commit (including one other place in the file).
| result |= CorInfoFlag.CORINFO_FLG_BEFOREFIELDINIT; | ||
| { | ||
| #if READYTORUN | ||
| if (_compilation.CompilationModuleGroup.VersionsWithType(metadataType)) |
There was a problem hiding this comment.
It may be better to clear the flag CORINFO_FLG_BEFOREFIELDINIT at the end instead. One less #if READYTORUN ifdef and one less call to VersionsWithType.
There was a problem hiding this comment.
Nice, thanks for the suggestion, fixed in 3rd commit.
ghost
locked as resolved and limited conversation to collaborators
I originally tried to use the flag CORINFO_FLG_CUSTOMLAYOUT
as suggested by JanK but that doesn't work - apparently the code at
runtime/src/coreclr/src/jit/lclvars.cpp
Line 1696 in 57a931b
the CUSTOMLAYOUT flag is only honored for HFA fields.
Thanks
Tomas
/cc @dotnet/crossgen-contrib