SPMI: Fixes to record-and-replay workflows#125981
Merged
jakobbotsch merged 5 commits intodotnet:mainfrom Mar 30, 2026
Merged
Conversation
Three fixes: - Skip reading compile results for superpmi replay. If the collection has compile results we never want to reuse them for superpmi replay. Various recording functions do not work properly when there already are existing compile results -- e.g. `recAllocMem` will keep the old instruction buffer instead of the new one from the replay. - Support passing a directory to -mch_files, and allow .mc files passed to -mch_files - Fix diffs report generation when a collection has zero succeeding contexts. Previously we would hit a division-by-zero error.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves SuperPMI record/replay robustness by preventing reuse of stored compile results during replay, expanding -mch_files input support (directories and .mc files), and hardening summary generation when collections contain no successful contexts.
Changes:
- Add a
readCompileResultsswitch toMethodContext::Initializeand disable compile-result loading for replay paths. - Extend
superpmi.pyto accept directories for-mch_filesand include.mcalongside.mchin discovery. - Avoid division-by-zero in diffs summary generation when total contexts is zero.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/superpmi/superpmi/superpmi.cpp | Disable reading compile results during replay and reset compile-result state. |
| src/coreclr/tools/superpmi/superpmi/streamingsuperpmi.cpp | Disable reading compile results when loading contexts for streaming replay. |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontextreader.cpp | Explicitly request compile-result loading where needed (hash workflow). |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontextiterator.cpp | Explicitly request compile-result loading for iterator-based reads. |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h | Add readCompileResults parameter to initialization helpers/overloads. |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp | Gate compile-result map deserialization on readCompileResults. |
| src/coreclr/tools/superpmi/mcs/verbstrip.cpp | Use the new readCompileResults flag to implement CR stripping. |
| src/coreclr/scripts/superpmi.py | Accept directories for -mch_files, include .mc, and guard percentage computations. |
Comments suppressed due to low confidence (1)
src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp:188
- The updated static Initialize(unsigned char* ...) overload takes readCompileResults, but the implementation still calls the instance Initialize(mcIndex, buff, size) without passing it. This won’t compile and also ignores the caller’s intent. Pass readCompileResults through to the instance Initialize call.
{
MethodContext* mc = new MethodContext();
mc->index = mcIndex;
*ppmc = mc;
return mc->Initialize(mcIndex, buff, size);
Member
Author
|
PTAL @dotnet/jit-contrib |
Member
Author
|
@EgorBo Can you take a look? |
EgorBo
approved these changes
Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes:
Skip reading compile results for superpmi replay. If the collection has compile results we never want to reuse them for superpmi replay. Various recording functions do not work properly when there already are existing compile results -- e.g.
recAllocMemwill keep the old instruction buffer instead of the new one from the replay.Support passing a directory to -mch_files, and allow .mc files passed to -mch_files
Fix diffs report generation when a collection has zero succeeding contexts. Previously we would hit a division-by-zero error.