-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Create a summary Markdown file for all asm diffs #54430
Create a summary Markdown file for all asm diffs #54430
Conversation
Create a per-MCH file summary.md file, then accumulate them all into a single overall summary Markdown file, for use in GitHub comments. Uses the existing `jit-analyze --md` functionality. Also, change asm diffs to not report missing data or asm diffs as a replay failure. Finally, don't overwrite superpmi.log files (or the new diff_summary.md files): create new, unique file names for each run.
@kunalspathak @dotnet/jit-contrib PTAL |
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.
Minor comments.
# Don't report as replay failure asm diffs (return code 2) or missing data (return code 3). | ||
# Anything else, such as compilation failure (return code 1, typically a JIT assert) will be | ||
# reported as a replay failure. | ||
if return_code != 2 and return_code != 3: |
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.
This is superpmi replay, so why would it get return_code == 2
(asm diffs failure)?
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.
This is the asm diffs case (only).
I'll go ahead and add similar "ignore" code to the replay
case, but only for missing data.
src/coreclr/scripts/superpmi.py
Outdated
summary_mch_filename = os.path.basename(summary_mch) # Display just the MCH filename, not the full path | ||
summary_file = summary_file_info[1] | ||
with open(summary_file, "r") as read_fh: | ||
write_fh.write(summary_mch_filename + ":\n\n") |
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.
write_fh.write(summary_mch_filename + ":\n\n") | |
write_fh.write("##" + summary_mch_filename + ":" + os.linesep + os.linesep) |
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.
According to the docs (https://docs.python.org/3/library/os.html#os.linesep), you shouldn't use os.linesep when writing in text mode: Python auto-converts.
The ##
is a good idea.
Create a per-MCH file summary.md file, then accumulate them all into
a single overall summary Markdown file, for use in GitHub comments.
Uses the existing
jit-analyze --md
functionality.Also, change asm diffs to not report missing data or asm diffs as a
replay failure.
Finally, don't overwrite superpmi.log files (or the new diff_summary.md files):
create new, unique file names for each run.