Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SPMI no ASMDiff between Checked and Release #61335

Merged
merged 17 commits into from
Mar 2, 2022
Merged

SPMI no ASMDiff between Checked and Release #61335

merged 17 commits into from
Mar 2, 2022

Conversation

JulieLeeMSFT
Copy link
Member

Created the first file that builds release and checked builds

@ghost
Copy link

ghost commented Nov 9, 2021

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Created the first file that builds release and checked builds

Author: JulieLeeMSFT
Assignees: -
Labels:

area-Infrastructure-coreclr
Milestone: -

@JulieLeeMSFT
Copy link
Member Author

Fixes #49949.

@JulieLeeMSFT JulieLeeMSFT self-assigned this Nov 13, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Nov 13, 2021
@JulieLeeMSFT JulieLeeMSFT force-pushed the 49949 branch 2 times, most recently from 6eba786 to 2b3269f Compare December 2, 2021 20:06
@ghost
Copy link

ghost commented Jan 1, 2022

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@ghost ghost closed this Jan 1, 2022
@JulieLeeMSFT JulieLeeMSFT reopened this Jan 26, 2022
@JulieLeeMSFT JulieLeeMSFT marked this pull request as ready for review January 28, 2022 01:34
@JulieLeeMSFT
Copy link
Member Author

@BruceForstall PTAL.
cc @dotnet/jit-contrib

@BruceForstall
Copy link
Member

Pipeline is https://dev.azure.com/dnceng/public/_build?definitionId=1068

BruceForstall
BruceForstall requested changes Jan 29, 2022
View reviewed changes
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an open question here about whether the existing superpmi asmdiffs pipeline py scripts (and maybe .proj file and .yml files) should be extended with additional flags/options, and the checked/release case should share them, since there is so much duplicated code. That would, of course, make the files more complicated, as they handle more cases.

Another option: this code is focused on checked/release (which is really what we want). A slight generalization is to handle release/release, or even debug/release. Basically, any asm diffs where one or more of the JITs is Release. The key is that a release JIT can't generate textual .dasm files.

src/coreclr/scripts/superpmi.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi_asmdiffs_checked_release.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi_asmdiffs_checked_release.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi_asmdiffs_checked_release.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi_asmdiffs_checked_release.py Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi_asmdiffs_checked_release.py Outdated Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jan 29, 2022
@JulieLeeMSFT
Copy link
Member Author

Hi @BruceForstall, it is ready for review.

The pipeline caught two asm diffs - Link.

  • unix-arm64
    -- ISSUE: <ASM_DIFF> main method 246505 of size 2418 differs
  • win-arm64
    -- ISSUE: <ASM_DIFF> main method 249554 of size 2418 differs

@BruceForstall BruceForstall mentioned this pull request Feb 4, 2022
Closed
@BruceForstall
Copy link
Member

I opened #64793 to track the asm diff.

BruceForstall
BruceForstall requested changes Feb 8, 2022
View reviewed changes
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small suggestions

src/coreclr/scripts/superpmi.py Outdated Show resolved Hide resolved
eng/pipelines/coreclr/superpmi-asmdiffs-checked-release.yml Outdated Show resolved Hide resolved
src/coreclr/scripts/superpmi_asmdiffs_checked_release_setup.py Outdated Show resolved Hide resolved
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 8, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 16, 2022
@BruceForstall
Copy link
Member

Looks like there are new checked/release asmdiffs

@JulieLeeMSFT
Copy link
Member Author

Looks like there are new checked/release asmdiffs

pipelink link
asm diff in coreclr_tests.pmi.Linux.arm64.checked.mch

  • win-arm64 log:
    -- 323185e9-208a-4b35-a413-23f9aac2d5f7.windows.arm64\coreclr_tests.pmi.windows.arm64.checked.mch
    -- ISSUE: <ASM_DIFF> main method 249555 of size 2418 differs

  • unix-arm64 log:
    -- 323185e9-208a-4b35-a413-23f9aac2d5f7.Linux.arm64\coreclr_tests.pmi.Linux.arm64.checked.mch
    -- ISSUE: <ASM_DIFF> main method 246450 of size 2418 differs

The detected method numbers are different from the last diff.

@JulieLeeMSFT
SPMI no ASMDiff between Checked and Release
efc24d4 
Created the first file that builds release and checked builds
@JulieLeeMSFT
SuperPMI asmdiff checked and release
3a78a69 
* Added downloading JIT release builds in superpmi-asmdiff-job
* Added dummy 'run-superpmi-asmdiff-job'
@JulieLeeMSFT
SuperPmi asmdiff checked release
1959d1d 
* Added JIT release download
* Added dummy run job that just runs superpmi replay
@JulieLeeMSFT
SuperPMI asmdiff checked release
a9bad61 
* Remove the 4th argument for jobName and displayName
@JulieLeeMSFT
SuperPMI asmdiff checked release
d1a6cb6 
* In run-superpmi-asmdiff-checked-release-job.yml,
  * Added release_directory and release artifact to the script arguments
  * Added ReleaseCorrelationPayloadDirectory and ReleaseArtifactName to parameters to send-to-helix-step.yml

* In superpmi_asmdiff_checked_release_setup.py
  * Added release_directory and release_artifactname arguments
  * Copied release binaries to source_directory/payload_release
@JulieLeeMSFT
SuperPMI asmdiffs checked release
5c6dddf 
* Added superpmi_asmdiffs_checked_release_setup.py to set up Helix directories for checked in base and release in diff directories.
* Use superpmi_asmdiffs.py and superpmi-asmdiffs.proj.
* Renamed files from asmdiff to asmdiffs.
@JulieLeeMSFT
SuperPMI asmdiffs checked release: resolve merge conflict
16fdb6e
@JulieLeeMSFT
SuperPMI asmdiffs checked release: Fixed to use a proj and a script f…
3f80738 
…ile that compare with Release
@JulieLeeMSFT
Superpmi asmdiffs checked release: changed to --diff_with_release as …
84a456e 
…a boolean arg
@JulieLeeMSFT
SuperPMI asmdiffs checked release: Add dependency to completed Releas…
30e8de1 
…e build
@JulieLeeMSFT
Superpmi asmdiffs checked release: add diff_with_release back that go…
75b1d60 
…t removed from merge
@JulieLeeMSFT
SuperPMI asmdiffs checked release: Do not create asm file for asmdiff…
1fc1380 
… check with release
@JulieLeeMSFT
SuperPMI asmdiffs checked release: Report failure asm diffs (return c…
21af7b7 
…ode 2) if --diff_with_release
@JulieLeeMSFT
SuperPMI asmdiffs checked release: Handled code review feedback to re…
77328b6 
…move jit-analyze and diff_summary.md
@JulieLeeMSFT
SuperPMI asmdiffs checked release: Changed to checked_directory inste…
cf3ecf7 
…ad of base_directory
@JulieLeeMSFT
SuperPMI asmdiffs checked release: explictly say checked_directory
fd9ad6c
@JulieLeeMSFT
SuperPMI asmdiffs checked release: schedule to run on Saturdays and S…
8537008 
…undays only.
@JulieLeeMSFT
Copy link
Member Author

Failures are unrelated to this PR. Merging it.

@JulieLeeMSFT JulieLeeMSFT merged commit 7830fdd into dotnet:main Mar 2, 2022
@JulieLeeMSFT JulieLeeMSFT deleted the 49949 branch March 2, 2022 23:33
This was referenced Mar 2, 2022
Closed
Closed
@JulieLeeMSFT
Copy link
Member Author

@echesakovMSFT, I see many REL32 error from today's run. Bruce said that you fixed it, and you might know about this error. Any idea?
LInk
ERROR: REL32 relocation overflows field! delta=0xFFFFFFFF6646CB48

@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Mar 3, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BruceForstall BruceForstall BruceForstall approved these changes

Assignees

@JulieLeeMSFT JulieLeeMSFT

Labels
area-Infrastructure-coreclr
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@JulieLeeMSFT @BruceForstall