fix(codegen): sort closure-source emission so the same input compiles to the same IR (#7038) - #7039
Merged
Merged
Conversation
added 2 commits
July 30, 2026 06:15
… to the same IR emit_artifacts iterated hir.closure_source_text -- a HashMap -- when emitting retained closure-source constants, so the @.str.N numbering of those constants was a per-process permutation. Same input, different .ll on every run. The loop immediately above walks hir.functions (a Vec) and was already deterministic; only the materialized-closure loop keyed off map order. Collect, filter, sort by FuncId, then emit. Emission order is the only thing that changes. Why it matters beyond tidiness: it silently invalidates any A/B that compares raw LLVM IR. That technique underpins the planned codegen-perturbation measurement, and #7037 hit it directly -- its first observational-equivalence proof passed on nondeterministic output and had to be redone with a normalizer and four runs per arm. Evidence -- 6 compiles of one file, md5 of the concatenated per-module .ll: pristine main 5 distinct hashes across 6 runs this fix 1 hash across 6 runs Reported by the #7037 agent while proving --opt-report observational. Closes #7038.
📝 WalkthroughWalkthroughLLVM artifact generation now sorts retained closure-source entries by ChangesCodegen determinism
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-codegen/src/codegen/artifacts.rs (1)
1861-1877: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a separate-process determinism regression test.
The sorting fix is correct, but the PR objective calls for an automated check that invokes separate compiler processes and compares byte-identical output. The six-run measurement in the changelog is not an ongoing regression guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/codegen/artifacts.rs` around lines 1861 - 1877, Add a regression test covering the materialized closure emission path around materialized_closure_sources that invokes the compiler in separate processes multiple times with identical input, captures the generated output, and asserts the outputs are byte-for-byte identical. Keep the test focused on cross-process determinism so it guards the FuncId-based sorting fix rather than only repeated execution within one process.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/7039-codegen-determinism.md`:
- Around line 1-4: Update the changelog entry’s nondeterminism statement to use
probabilistic wording: replace the claim that the same input produces different
IR on every run with wording that it can produce different IR across runs.
Preserve the explanation about HashMap iteration and @.str.N numbering.
---
Nitpick comments:
In `@crates/perry-codegen/src/codegen/artifacts.rs`:
- Around line 1861-1877: Add a regression test covering the materialized closure
emission path around materialized_closure_sources that invokes the compiler in
separate processes multiple times with identical input, captures the generated
output, and asserts the outputs are byte-for-byte identical. Keep the test
focused on cross-process determinism so it guards the FuncId-based sorting fix
rather than only repeated execution within one process.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc809ffb-63ae-4a48-9873-0a859694731e
📒 Files selected for processing (2)
changelog.d/7039-codegen-determinism.mdcrates/perry-codegen/src/codegen/artifacts.rs
Comment on lines
+1
to
+4
| Fixed non-deterministic LLVM IR: `emit_artifacts` iterated `hir.closure_source_text` | ||
| (a `HashMap`) when emitting retained closure-source constants, so the `@.str.N` | ||
| numbering of those constants was a per-process permutation — the same input | ||
| compiled to different IR on every run. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use probabilistic wording for the nondeterminism claim.
HashMap iteration can vary across processes, but independent runs may still produce the same order. Replace “different IR on every run” with “can produce different IR across runs.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog.d/7039-codegen-determinism.md` around lines 1 - 4, Update the
changelog entry’s nondeterminism statement to use probabilistic wording: replace
the claim that the same input produces different IR on every run with wording
that it can produce different IR across runs. Preserve the explanation about
HashMap iteration and @.str.N numbering.
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.
Closes #7038.
What
emit_artifacts(codegen/artifacts.rs) iteratedhir.closure_source_text— aHashMap— when emitting retained closure-source constants. The@.str.Nnumbering of those constants was therefore a per-process permutation: the same input compiled to different LLVM IR on every run.The loop immediately above it walks
hir.functions(aVec) and was already deterministic; only the materialized-closure loop keyed off map iteration order. The fix collects, filters, sorts byFuncId, then emits. Emission order is the only thing that changes — the set of emitted constants and every other instruction is untouched.Why it is not cosmetic
It silently invalidates any A/B that compares raw LLVM IR — a technique several representation-selection and GC investigations have relied on for evidence.
PR #7037 hit it head-on. Its first proof that
--opt-reportwas observational was the obvious one: compile with the flag off, compile with it on, diff the.ll. That passed, and was not evidence — two runs can match by luck or differ for reasons unrelated to the change. It had to be redone with four runs per arm plus a content-canonicalising normalizer.It also matters for what comes next: the planned codegen-perturbation measurement (how many hot loops lose native regions to rooting calls) works by comparing emitted IR. That measurement is unsound while codegen is nondeterministic.
Evidence
6 compiles of one file (arrow functions in
map/filter/reduce/sort, so several materialized closures), md5 of the concatenated per-module.ll,PERRY_NO_AUTO_OPTIMIZE=1, same binary and same source in each arm:mainFalsifiable in both directions: if the diagnosis were wrong, the fixed arm would still show multiple hashes; if the file exercised no materialized closures, the unfixed arm would show one.
Scope
One function, one loop. No behaviour change to emitted code — only the order in which two string constants are written.
Note for reviewers:
linton this branchcargo fmt --all -- --checkfails on four files this PR does not touch, all pre-existing onmainatb5dbf3f63:They are deliberately not fixed here — that is a separate janitorial change and folding it in would obscure a one-loop diff. The file this PR touches is
rustfmt --checkclean.lintis in any case already red repo-wide on the stale public benchmark baseline, which fails before the fmt step.Summary by CodeRabbit