Version resilient cross module code compilation and inlining by davidwrighton · Pull Request #71271 · dotnet/runtime

davidwrighton · 2022-06-24T18:18:42Z

Refactor Module into ModuleBase and Module
- The goal is to have allow a subset version of Module which can only hold refs, this is to be used by the manifest module in an R2R image to allow for version resilient cross module references.
- Update handling of ModuleBase so that its used everywhere that tokens are parsed from R2R
- Remove ENCODE_MODULE_ID_FOR_STATICS and ENCODE_ACTIVE_DEPENDENCY
  - These were only used for NGEN, and conflict with easy impelmentation for the ModuleBase concept
- Remove locking in ENCODE_STRING_HANDLE processing, and tweak comments. Comments applied to the removed ngen based code, and the lock was only necessary for the old ngen thing.
- Adjust ComputeLoaderModuleWorker for locating loader module
  - Follow comment more accurately, to avoid putting every generic into its definition module. This will make R2R function lookup able to find compiled instantiations in some cases. This may be what we want long term, it may not.
- Remove MemberRefToDesc map and replace with LookupMap like the other token types. We no longer make use of the hot table, so this is more efficient
  - Also reduces complexity of implementation of ModuleBase
Build fixup to describe a single method as a standalone blob of data
- There are parallel implementations in Crossgen2 and in the runtime
- They produce binary identical output
- Basic R2RDump support for new fixup
Adjust module indices used within the R2R format to support a module index which refers to the R2R manifest metadata. This requires bumping the R2R version to 6.2
- Add a module index between the set of assembly refs in the index 0 module and the set of assembly refs in the R2R manifest metadata
Adjust compilation dependency rules to include a few critical AsyncStateMachineBox methods
Remove PEImage handling of native metadata which was duplicative
Do not enable any more devirtualization than was already in use, even in the cross module compilation scenario. In particular, do not enable devirtualization of methods where the decl method isn't within the version bubble, even if the decl method could be represented with a cross module reference token. (This could be fixed, but is out of scope for this initial investigation)

Make the compilation deterministic in this new model, even though we are generating new tokens on demand

Implement this by detecting when we need new tokens during a compile, and recompiling with new tokens when necessary
This may result in compiling the same code as much as twice

Compile the right set of methods with cross module inlining enabled

Add support for compiling the called virtual methods on generic types
- This catches the List and Dictionary<TKey,TValue> scenarios
Support input of PGO data to control the set of methods
Enable new READYTORUN_FLAG_UNRELATED_R2R_CODE flag on R2R images which is used to indicate which modules may have generic code not directly related to the metadata of the image
Lookup R2R methods in an alternate location as well as the metadata defining module. This allows for many generics to be embedded without needing to use the new READYTORUN_FLAG_UNRELATED_R2R_CODE flag, which has global effects on performance.
Add command line switches to enable/disable the new behavior
Enhance the verion resilience test to cover this new behavior

…e generic compilation to Crossgen2 (dotnet#68919)" (dotnet#71076)" This reverts commit 89dc073.

New command line option: --opt-cross-module-pgo-module which adds modules where if they are specified via this switch, and there is PGO data then, methods with generics involving this module will be cross module inline eligible. This adds a requirement that loads of the specified modules must be loadable at any time during method resolution from the POV of the defined module. New handling in the runtime loader for cross module token loads from directly allowable extra modules as well as dependent loads. - The difference is that the dependent stuff should not be able to force load during the ILBody resolution phase of the fixup logic(Not yet implemented), and that the assembly is not necessarily directly loadable from the module that was compiled as R2R.

…odule_take3

- The version resilience test had 2 major bugs in it 1. It was not properly specifying references, so references to other dlls were not being handled 2. It was generating .ni.dll files, which are not actually loaded by the runtime anymore, so even for the code it did generate, the cross module references were not in use. - In addition, fixing the build, caused us to generate an assertion when handling fields of sequential/explicit types from another module, where we did not generate a correct cross module version resilient fixup. (That's what the fix is in CorInfoImpl.ReadyToRun.cs) Disable pinvoke inlining across version bubbles Address assertion around byte array load of R2R file on unix

cshung · 2022-07-01T01:21:07Z

@@ -264,6 +276,8 @@ fixup kind, the rest of the signature varies based on the fixup kind.
 | READYTORUN_FIXUP_Verify_TypeLayout       | 0x32 | Generate a runtime check to ensure that the field offset matches between compile and runtime. Unlike CheckFieldOffset, this will generate a runtime exception on failure instead of silently dropping the method
 | READYTORUN_FIXUP_Check_VirtualFunctionOverride | 0x33 | Generate a runtime check to ensure that virtual function resolution has equivalent behavior at runtime as at compile time. If not equivalent, code will not be used. See [Virtual override signatures](virtual-override-signatures) for details of the signature used.
 | READYTORUN_FIXUP_Verify_VirtualFunctionOverride | 0x33 | Generate a runtime check to ensure that virtual function resolution has equivalent behavior at runtime as at compile time. If not equivalent, generate runtime failure. See [Virtual override signatures](virtual-override-signatures) for details of the signature used.


Yes. that should be fixed.

…on of R2R versioning test. Its not needed and given the existence of bug 71507 is a problem

MichalStrehovsky · 2022-07-01T01:51:13Z

                    Console.WriteLine($@"Duplicate symbol - 2nd occurrence: [{nodeIndex}:{symbolIndex}], {definedSymbol.GetMangledName(_nodeFactory.NameMangler)}");
-                    Debug.Fail("Duplicate node name emitted to file",
-                    $"Symbol {definedSymbol.GetMangledName(_nodeFactory.NameMangler)} has already been written to the output object file {_objectFilePath} with symbol {alreadyWrittenSymbol}");
+                 //   Debug.Fail("Duplicate node name emitted to file",


This looks like a useful assertion. Did you intend to leave it commented out?

Sigh, yes, I'll turn this assert back on. I found a place where our name mangler wasn't completely competent at generating unique names. As I recall, it caused problems compiling Roslyn with the optimizations entirely enabled, but it was two method fixups which were very subtly distinct in their behavior.

It's not a problem for crossgen in reality, as duplicate symbols are not actually a fault, but as we share a name mangler with NativeAOT it's probably worth chasing down sooner rather than later.

MichalStrehovsky · 2022-07-01T02:11:41Z

+            {
+                if (_asyncStateMachineBox == null)
+                {
+                    _asyncStateMachineBox = SystemModule.GetType("System.Runtime.CompilerServices", "AsyncTaskMethodBuilder`1").GetNestedType("AsyncStateMachineBox`1");


Nit: We have a GetKnownType and GetKnownNestedType extension method that throws "nicer" exceptions when things are missing.

MichalStrehovsky · 2022-07-01T02:21:51Z

+
+                if (localsBlob.Length == 0)
+                {
+                    // No locals. Encode a 0 to indicate this


Could we just not write anything? I assume the length of the signature is part of the comparison at runtime, otherwise we would compare a method with no locals (0 written here) and a method with skiplocalsinit (0 written below) as equal.

I want to be able to theoretically parse this thing in R2RDump one day. I'll change the encoding to encode a 2, which is reliably unique.

davidwrighton · 2022-07-01T04:28:21Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2022-07-01T04:28:39Z

Azure Pipelines successfully started running 1 pipeline(s).

cshung · 2022-07-01T17:57:06Z

        OwnerCompositeExecutable = 116, // Added in 4.1
        PgoInstrumentationData = 117, // Added in 5.2
        ManifestAssemblyMvids = 118, // Added in 5.3
+        CrossModuleInlineInfo = 119, // Added in 6.2


Added in 6.3?

Ah, yes. Jakob added 6.2 after I wrote this doc and I missed updating this detail.

…was merged

davidwrighton added 5 commits June 21, 2022 14:14

Revert "Revert "Add support for cross module inlining and cross modul…

e5c46e5

…e generic compilation to Crossgen2 (dotnet#68919)" (dotnet#71076)" This reverts commit 89dc073.

Fix issue with hash table critical section

f5051f1

Add PGO support, and rework command line arguments to new set

54814d7

Make it all work right

8d8a18c

ghost assigned davidwrighton Jun 24, 2022

ghost added the area-ReadyToRun label Jun 24, 2022

davidwrighton added 8 commits June 24, 2022 14:31

Merge branch 'main' of https://github.com/dotnet/runtime into cross_m…

5c0e46c

…odule_take3

Cleanup known problems in prep for checkin

f634742

Enable more thorough version resilience testing

0eaf2aa

Cross module version bubble test is now working

9997600

Revert change which adjusted peimagelayout behavior

361cb09

Disable R2R load test in the version resilience test

d16c105

Disable opt-cross-module by default

d28591f

davidwrighton changed the title ~~Cross module take3~~ Version resilient cross module code compilation and inlining Jun 30, 2022

davidwrighton marked this pull request as ready for review June 30, 2022 21:47

davidwrighton requested a review from MichalStrehovsky as a code owner June 30, 2022 21:47

cshung reviewed Jul 1, 2022

View reviewed changes

davidwrighton added 2 commits June 30, 2022 18:45

Remove TestLoadR2RImageFromByteArray from cross module inlining porti…

e7a90a2

…on of R2R versioning test. Its not needed and given the existence of bug 71507 is a problem

Merge branch 'main' of github.com:dotnet/runtime into cross_module_take3

2cf7836

MichalStrehovsky reviewed Jul 1, 2022

View reviewed changes

cshung reviewed Jul 1, 2022

View reviewed changes

Comment thread src/coreclr/inc/corcompile.h

Address code review feedback

6852505

cshung reviewed Jul 1, 2022

View reviewed changes

cshung approved these changes Jul 1, 2022

View reviewed changes

More code review feedback

2ac5a0a

davidwrighton merged commit a78c46d into dotnet:main Jul 1, 2022

gbalykov mentioned this pull request Jul 2, 2022

crossgen2 fails to compile dlls on armel with versionbubble after compilation of ILCompiler.ReadyToRun.dll #55663

Closed

tommcdon mentioned this pull request Jul 7, 2022

ICorProfilerInfo6::EnumNgenModuleMethodsInliningThisMethod returns no inlined methods for System.Private.CoreLib #71774

Closed

davidwrighton added a commit to davidwrighton/runtime that referenced this pull request Jul 7, 2022

Tweak inline reporting rules to behave as they did before dotnet#71271 …

07dc1a2

…was merged

ghost locked as resolved and limited conversation to collaborators Aug 1, 2022

davidwrighton deleted the cross_module_take3 branch April 13, 2023 18:53

Navigation Menu

Uh oh!

Version resilient cross module code compilation and inlining - #71271

Version resilient cross module code compilation and inlining#71271
davidwrighton merged 17 commits into
dotnet:mainfrom
davidwrighton:cross_module_take3

davidwrighton commented Jun 24, 2022 •

edited

Loading

Uh oh!

cshung Jul 1, 2022

Uh oh!

davidwrighton Jul 1, 2022

Uh oh!

MichalStrehovsky Jul 1, 2022

Uh oh!

davidwrighton Jul 1, 2022

Uh oh!

davidwrighton Jul 1, 2022

Uh oh!

MichalStrehovsky Jul 1, 2022

Uh oh!

MichalStrehovsky Jul 1, 2022

Uh oh!

davidwrighton Jul 1, 2022

Uh oh!

Uh oh!

davidwrighton commented Jul 1, 2022

Uh oh!

azure-pipelines Bot commented Jul 1, 2022

Uh oh!

cshung Jul 1, 2022

Uh oh!

davidwrighton Jul 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

davidwrighton commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidwrighton commented Jul 1, 2022

Uh oh!

azure-pipelines Bot commented Jul 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidwrighton commented Jun 24, 2022 •

edited

Loading