-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Make it possible to order methods by specifying symbolic names #117537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Text file with an ordered list of symbol names is much easier to emit based on sampling than MIBC (that requires methods in IL terms). Methods that are listed are generated in the specified order. Methods that are not listed are sorted by the compiler. Things that are listed but don't exist are silently skipped.
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for ordering emitted methods by providing a symbol list file, plus a corresponding smoke test and build integration.
- Adds a new
--order
option to ILCompiler and wires it through to the RyuJIT builder and file layout optimizer. - Implements an explicit layout algorithm in
FileLayoutOptimizer
that reads symbol names from the specified order file. - Updates the test suite with
Ordering.cs
,order.txt
, and MSBuild targets to exercise and pass the order file to the native AOT compilation.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/tests/nativeaot/SmokeTests/UnitTests/order.txt | New order file listing symbols for explicit layout |
src/tests/nativeaot/SmokeTests/UnitTests/UnitTests.csproj | Added <IlcOrderFile> property and include Ordering.cs |
src/tests/nativeaot/SmokeTests/UnitTests/Ordering.cs | Smoke test sorting method pointers by address |
src/tests/nativeaot/SmokeTests/UnitTests/Main.cs | Added Ordering.Run to the test sequence |
src/coreclr/tools/aot/ILCompiler/Program.cs | Integrated .UseSymbolOrder into the compilation builder |
src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs | Defined the --order CLI option |
src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilationBuilder.cs | Added UseSymbolOrder and _orderFile field |
src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs | Propagated orderFile to FileLayoutOptimizer |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/FileLayoutOptimizer.cs | Implemented Explicit layout by reading the order file |
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets | Passed --order and --method-layout:explicit and included order file in inputs |
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/FileLayoutOptimizer.cs:208
- There are no existing tests validating that the
Explicit
layout algorithm actually applies the file's symbol order; consider adding unit or integration tests that inspect the generated layout to ensure the feature works as intended.
case MethodLayoutAlgorithm.Explicit:
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/FileLayoutOptimizer.cs:223
- [nitpick] Normalize or trim symbol names (e.g. strip leading underscores) when comparing against
GetMangledName
to ensure cross-platform consistency and avoid silent skips.
string symbolName = sr.ReadLine();
order[methods[i]] = int.MaxValue; | ||
} | ||
|
||
using (StreamReader sr = new StreamReader(_orderFile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against null or empty _orderFile
before creating the StreamReader; if the path is not set or file is missing, provide a clear exception or skip explicit ordering to avoid an unhelpful runtime error.
Copilot uses AI. Check for mistakes.
@@ -361,6 +361,7 @@ public int Run() | |||
string compilationUnitPrefix = multiFile ? Path.GetFileNameWithoutExtension(outputFilePath) : ""; | |||
var builder = new RyuJitCompilationBuilder(typeSystemContext, compilationGroup) | |||
.FileLayoutAlgorithms(Get(_command.MethodLayout), Get(_command.FileLayout)) | |||
.UseSymbolOrder(Get(_command.OrderFile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only call UseSymbolOrder
when the --order
option was actually provided (e.g. check result.HasOption(OrderFile)
), otherwise you may pass an empty or null path into the builder.
Copilot uses AI. Check for mistakes.
@@ -322,7 +324,7 @@ The .NET Foundation licenses this file to you under the MIT license. | |||
</Target> | |||
|
|||
<Target Name="IlcCompile" | |||
Inputs="@(IlcCompileInput);@(IlcReference);@(IlcSatelliteAssembly);@(RdXmlFile);%(ManagedBinary.IlcRspFile)" | |||
Inputs="@(IlcCompileInput);@(IlcReference);@(IlcSatelliteAssembly);@(RdXmlFile);$(IlcOrderFile);%(ManagedBinary.IlcRspFile)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Make inclusion of $(IlcOrderFile)
in the Inputs
attribute conditional (similar to the IlcArg
conditions) to prevent unnecessary rebuilds when no order file is set.
Inputs="@(IlcCompileInput);@(IlcReference);@(IlcSatelliteAssembly);@(RdXmlFile);$(IlcOrderFile);%(ManagedBinary.IlcRspFile)" | |
Inputs="@(IlcCompileInput);@(IlcReference);@(IlcSatelliteAssembly);@(RdXmlFile);%(ManagedBinary.IlcRspFile)" | |
Inputs="@(IlcCompileInput);@(IlcReference);@(IlcSatelliteAssembly);@(RdXmlFile);%(ManagedBinary.IlcRspFile);$(IlcOrderFile)" Condition="'$(IlcOrderFile)' != ''" |
Copilot uses AI. Check for mistakes.
// Method addresses are not observable in WASM | ||
if (OperatingSystem.IsWasi() || OperatingSystem.IsBrowser()) | ||
return 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making our downstream lives easier :).
Now, if anyone wanted to know a bit more about how this works on WASM...
These ordering options are not too useful in WASM since it is of course a virtual machine and the method order of the final machine code doesn't need to reflect the WASM binary method order. However, there are cases (e. g. wasmtime) where these orders are indeed the same, so it's not useless either.
The WASM linker doesn't allow you to explicit 'order' methods since it processes each one as a single unit (a'la the Apple linker with its subsections). So in theory it can reorder them as it likes. In practice the order from the object files is preserved as an implementation detail.
Finally, the ldftn
operation in WASM produces table (of all methods that could be called indirectly) indices, of which the order depends on the linker as well. I don't recall if it follows the same order as in the object files in practice.
There is still a way to observe the "function index", which corresponds to the in-WASM-file-position, via special relocations [or a JS-only API]. So this test is not completely unimplementable on WASM, it would need a compiler intrinsic though.
Text file with an ordered list of symbol names is much easier to emit based on sampling than MIBC (that requires methods in IL terms). Methods that are listed are generated in the specified order. Methods that are not listed are sorted by the compiler. Things that are listed but don't exist are silently skipped.
Cc @dotnet/ilc-contrib