SPMI: Run 64-bit diffs on OSX.14.Arm64.Open #116299
Merged
Conversation
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
This should make the 64-bit diffs quite a bit faster because: 1. The existing Windows queues are Azure managed, so they often need to scale up as part of running Helix work items, which adds time 2. The OSX queues are on-prem queues and always have machines ready to accept work. Additionally each machine is very powerful.
github-actions
bot
added
the
area-CodeGen-coreclr
This reverts commit 2d79c09.
This reverts commit 1c235f0.
BRANCH26 relocs were handled by seeing if we can fit a relative offset to the actual target in the instruction, and if so, doing that. Otherwise we would hardcode a jump to the end of the code. It turns out that outside macOS we never hit the former case, so we always hardcode a jump to the end of the code section. That makes diffing work outside macOS. On macOS, it happens rarely that the target (that comes from the original SPMI collection) is actually within range of the code that was allocated during SPMI replay. When that happens we end up with a two different immediates provided to the instruction by the base and the diff compilers, and hence diffing fails. Now, `NearDiffer::compareOffsets` actually has code that tries to compensate for relative offsets like this, by seeing if the absolute offset computed by "address + instrLen + immediate" is the same for the base and the diff compilers. However, it turns out that LLVM does not scale the immediate in the b/bl instruction representation. So for the above to work, it would actually need to be something like `instrLen + immediate << 2`. We could do that, but it seems better to align this with x64, which just skips all of the troubles by hardcoding the lower bits of the absolute offsets as if it was the relative offset.
|
PTAL @dotnet/jit-contrib
There is still some variance in the time it takes, which seems to be because the macOS 14 queue has machines of different performance, so if the slowest work items end up on the slowest machines, it takes 30+ minutes. But this is still way better than using the existing Windows queue which frequently has 30+ minute waits before the work items even start running. |
There was a problem hiding this comment.
Pull Request Overview
This PR aims to improve the performance of 64-bit diffs by leveraging on-prem OSX queues and removing obsolete Windows special casing for superpmi-collect.
- Removed redundant Windows-specific diff handling in superpmi_diffs_summarize.py.
- Updated platform and archive handling in superpmi_diffs_setup.py for OSX and adjusted partition logic.
- Modified pipeline definitions to include OSX arm64 and updated build parameters for superpmi diffs.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/scripts/superpmi_diffs_summarize.py | Removed unused target_windows and asmdiffs logic. |
| src/coreclr/scripts/superpmi_diffs_setup.py | Added macOS detection, updated JIT file matching and partition building logic. |
| src/coreclr/scripts/superpmi_diffs.py | Updated host OS/arch determination using defaults. |
| eng/pipelines/coreclr/templates/jit-replay-pipeline.yml | Updated buildArgs to reference the revised JIT set. |
| eng/pipelines/coreclr/templates/helix-queues-setup.yml | Adjusted helix queue conditions for superpmi-diffs. |
| eng/pipelines/coreclr/superpmi-diffs.yml | Included osx_arm64 platform, refined build steps and set helixQueueGroup to superpmi-diffs. |
Comments suppressed due to low confidence (3)
src/coreclr/scripts/superpmi_diffs_setup.py:246
- Consider adding a brief inline comment to explain why the additional check for do_asmdiffs is applied when determining the target platforms. This will help maintainers understand the logic behind selecting targets on non-Windows platforms.
if not target_windows and not do_asmdiffs:
eng/pipelines/coreclr/templates/jit-replay-pipeline.yml:35
- [nitpick] Please verify that switching the buildArgs to incorporate 'clr.alljitscommunity' is the intended behavior and that it aligns with the goals of the PR to improve diff performance.
buildArgs: -s clr.alljitscommunity+clr.spmi -c $(_BuildConfig)
eng/pipelines/coreclr/superpmi-diffs.yml:62
- [nitpick] Consider adding an inline comment explaining why the additional step to build CLR assets for x64 is necessary in the superpmi-diffs pipeline. This clarification will aid future maintainers in understanding the dependency on an x64 mcs.
- template: /eng/pipelines/common/templates/global-build-step.yml
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This should make the 64-bit diffs quite a bit faster because:
Also remove the SPMI special casing from windows-x64. It exists for superpmi-collect and was added in #74961 because at that point the other testing ran on multiple queues. Since that's not the case anymore I do not think we need it.