Reuse crossgen2 logic to lay out methods using PGO data#114736
Reuse crossgen2 logic to lay out methods using PGO data#114736MichalStrehovsky merged 2 commits intodotnet:mainfrom
Conversation
ghost
added
the
There was a problem hiding this comment.
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj: Language not supported
- src/coreclr/tools/aot/ILCompiler.RyuJit/ILCompiler.RyuJit.csproj: Language not supported
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunFileLayoutOptimizer.cs:321
- [nitpick] Consider using a structured logging API instead of directly writing to logger.Writer, to ensure consistent logging behavior and easier log management.
_logger.Writer.WriteLine("Warning: no call graph data was found or a .mibc file was not specified. Skipping Pettis Hansen method ordering.");
src/coreclr/tools/Common/Compiler/DependencyAnalysis/SortableDependencyNode.cs:152
- The removal of the partial method ApplyCustomSort in favor of directly comparing the CustomSort field appears consistent; please ensure that no scenarios relying on the legacy partial method behavior (especially under conditional compilation) are affected.
static partial void ApplyCustomSort(SortableDependencyNode x, SortableDependencyNode y, ref int result);
src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilationBuilder.cs
Show resolved
Hide resolved
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
| binaries are up to date and which are stale. --> | ||
| <GenerateDependencyFile>false</GenerateDependencyFile> | ||
| <Configurations>Debug;Release;Checked</Configurations> | ||
| <RunAnalyzers>false</RunAnalyzers> |
There was a problem hiding this comment.
Matches ILCompiler.ReadyToRun, there are several low value warnings and I'm honestly growing tired of the analyzers and pragma warning suppresions we're accumulating as a result and decided to just take a page from the crossgen project.
| new("--optimize-time", "--Ot") { Description = "Enable optimizations, favor code speed" }; | ||
| public Option<string[]> MibcFilePaths { get; } = | ||
| new("--mibc", "-m") { DefaultValueFactory = _ => Array.Empty<string>(), Description = "Mibc file(s) for profile guided optimization" }; | ||
| public Option<ReadyToRunMethodLayoutAlgorithm> MethodLayout { get; } = |
There was a problem hiding this comment.
Do these options have the right default? Or is that a future item?
There was a problem hiding this comment.
This is a copypaste from crossgen - this should default to 0, which means don't do it.
| <Compile Include="..\ILCompiler.ReadyToRun\Compiler\PettisHansenSort\CallGraphNode.cs" Link="Compiler\PettisHansenSort\CallGraphNode.cs" /> | ||
| <Compile Include="..\ILCompiler.ReadyToRun\Compiler\PettisHansenSort\DisjointSetForest.cs" Link="Compiler\PettisHansenSort\DisjointSetForest.cs" /> | ||
| <Compile Include="..\ILCompiler.ReadyToRun\Compiler\PettisHansenSort\PettisHansen.cs" Link="Compiler\PettisHansenSort\PettisHansen.cs" /> | ||
| <Compile Include="..\ILCompiler.ReadyToRun\Compiler\ReadyToRunFileLayoutOptimizer.cs" Link="Compiler\ReadyToRunFileLayoutOptimizer.cs" /> |
There was a problem hiding this comment.
It would be nice to drop ReadyToRun prefix for this.
There was a problem hiding this comment.
Sure, just wanted to keep the diff small
Fixes #114093.
Cc @dotnet/ilc-contrib