fix(codegen): retry oversized RS4GC functions with shadow roots, and CI hardening - #8696
Merged
Conversation
…CI hardening Lands #8687, #8686 and #8684. #8687 (closes #8679) replaces the post-RS4GC instruction-budget hard refusal with a typed spill-retry: an already-lowered LlFunction switches from native statepoint roots to a complete precise shadow frame and the unit is rebuilt at the originally requested optimization level. This is the durable handling for the estimator misses #8678 could only make more accurate. Retry termination is guaranteed rather than argued. `request_shadow_frame_spill()` latches on `force_shadow_frame` and returns false if already set; `apply_budget_spill_retry` records only the functions where it returned true, and any violation not recorded becomes a hard error naming it. A function therefore cannot be retried twice. #8686 builds the gap suite's fast-mode archives once in a dedicated `gap-suite-build` job and shares them across the six shards. The skipped-vs-failed distinction is handled explicitly: `always()` keeps a SKIPPED build (full mode, where the job never runs) from cascading into skipped shards, while the guard still requires `success` or `skipped`, so a genuine build FAILURE stops the shards. The shard also verifies the downloaded binary is runnable and exports PERRY_BIN / PERRY_RUNTIME_DIR. #8684 inlines dtolnay/rust-toolchain and SHA-pins every other third-party action. Verified no job and no gate is dropped: the single removed step is the fast-mode archive build, which moved into gap-suite-build. No version bump.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (39)
📝 WalkthroughWalkthroughChangesRS4GC budget retry
CI workflow reproducibility
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant LLVMCompilation
participant RS4GCBudget
participant ShadowFrameLowering
participant NativeEmission
LLVMCompilation->>RS4GCBudget: enforce rewritten-function instruction cap
RS4GCBudget-->>LLVMCompilation: return typed budget violations
LLVMCompilation->>ShadowFrameLowering: request spill for violating functions
ShadowFrameLowering-->>LLVMCompilation: mutate lowering state
LLVMCompilation->>NativeEmission: rebuild and emit compiled output
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
Lands #8687, #8686 and #8684.
#8687 — RS4GC budget spill-retry (closes #8679)
Replaces the post-RS4GC instruction-budget hard refusal with a typed spill-retry: an already-lowered
LlFunctionswitches from native statepoint roots to a complete precise shadow frame, and the unit is rebuilt at the originally requested optimization level. This is the durable handling for the estimator misses that #8678 could only make more accurate.Retry termination is proven, not argued — the thing I most wanted to check before landing a retry loop:
apply_budget_spill_retryinserts intochangedonly when that returnedtrue, then hard-errors on any violation not inchanged("or were already retried"). A function that has already been spilled therefore returnsfalse→ lands inmissing→ fails loudly. It cannot be retried twice.#8686 — share the gap suite's fast-mode build across shards
Builds the archives once in a dedicated
gap-suite-buildjob. The skipped-vs-failed distinction — which has bitten this repo before, since a skipped job satisfies a dependency — is handled explicitly and correctly:always()keeps a SKIPPED build (full mode, where the job never runs) from cascading into skipped shards, while the guard still requiressuccessorskipped— so a genuine build failure stops the shards rather than letting them run vacuously. The download step is itself mode-gated, the shard verifies the downloaded binary is runnable, and it exportsPERRY_BIN/PERRY_RUNTIME_DIR(without which the wrapper exits 0 having run nothing).#8684 — inline
dtolnay/rust-toolchain, SHA-pin third-party actionsAudited for silently-dropped coverage, since that's the real risk in a CI-mechanics PR: no job removed, one job added (
gap-suite-build), and the single removed step is the fast-mode archive build — which moved into that job rather than disappearing.Validation (on the merged result)
lint-job checkers passperry-codegen --lib: 1202 passed, 0 failedperry-codegen --features llvm-inprocess --lib: 1202 passed, 0 failed (the feature fix(codegen): retry oversized RS4GC functions with shadow roots (#8679) #8687 actually changes)perry-runtime --lib(RUST_TEST_THREADS=1): 2655 passed, 0 failedperry --bin perry: 1031 passed, 0 failedNo version bump.
Summary by CodeRabbit
Bug Fixes
Chores