Conversation
A non-GC struct return buffer can be cleared in the prolog even though its first defining call overwrites it completely. Recognize that call as explicit initialization using the existing return-buffer definition helper and reference counts. Retain initialization for GC-containing buffers, partial definitions, promoted fields, async methods, and potentially observable exception paths. Validation against this change's base on updated main a652cdf: - Windows x64 Checked and Release and Linux x64 Release JIT builds. - 18 existing physical-promotion regression runs pass with normal, forced promotion and JitStress=2 settings. - Focused optimized, promotion-stress, JitStress=2 and warmed-up tiering/PGO executions pass on base and candidate; Tier1 is captured. - Windows codegen checks fail on base and pass on candidate. Linux behavior passes, with codegen checks where applicable. - Eleven SuperPMI collections: 983,234 comparable contexts; 238,457,491 -> 238,455,292 bytes (2,199 saved). 131 shrink; 7 grow by 36 bytes total. Zero compilation failures; 190 missing-recording contexts excluded. The 190 recording gaps affect both sides. - Release PIN JIT instructions, benchmarks: -0.03592%. - Release PIN JIT instructions, HashSet PGO: -0.01791%. Zeroing/layout tradeoffs include two 14-byte MakeDecimalLiteral increases with higher static PerfScore. The net figure includes all regressions. Code size, static PerfScore and JIT instruction counts do not establish application execution speed. No ARM64 or x86 execution is claimed. Related to dotnet#133833.
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Regression coverage does not yet exercise the new safeguards, and the project reference ordering nit remains.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR removes redundant JIT prolog initialization for fully defined, non-GC struct return buffers while preserving safety cases.
Changes:
- Recognizes eligible return-buffer calls as explicit initialization.
- Preserves initialization for partial, GC-containing, promoted, asynchronous, and observable exception cases.
- Adds regression/codegen tests and registers the test project.
File summaries
| File | Summary / review note |
|---|---|
src/tests/JIT/Directed/physicalpromotion/ReturnBufferInitialization.csproj |
Configures the return-buffer initialization tests. |
src/tests/JIT/Directed/physicalpromotion/ReturnBufferInitialization.cs |
Adds behavioral and codegen coverage; additional first-definition and GC-containing cases are needed to exercise the new safeguards. |
src/tests/JIT/Directed/Directed_do.csproj |
Registers the test project; its reference should be moved after readbackbeforeqmark to preserve ordering. |
src/coreclr/jit/optimizer.cpp |
Adds guarded recognition of eligible return-buffer initialization. |
Review details
Suppressed comments (1)
src/tests/JIT/Directed/Directed_do.csproj:12
- The
MergedWrapperProjectReferenceentries in this group are kept in path order (addressinterferencethroughreadbackbeforeqmark), but the newReturnBufferInitializationentry is inserted before them. Please move it afterreadbackbeforeqmarkso the list remains ordered.
<MergedWrapperProjectReference Include="physicalpromotion\ReturnBufferInitialization.csproj" />
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
Use SkipInit while retaining CLR local initialization so the exception, prior-read and partial-buffer cases reach their intended guards without a preceding explicit initialization store. Add a GC-containing return buffer whose callee collects before returning. Alphabetize the test project reference. Validation: JIT phase dumps confirm the guarded shapes and retained initialization. Windows baseline/candidate tests pass in normal, forced promotion, JitStress=2 and tiering/PGO modes. Linux baseline/candidate tests and Windows GCStress=0xC pass. Candidate codegen checks pass and baseline checks fail as expected. Release test-project build passes. Compiler logic is unchanged.
This branch has not been deployed
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
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.
A non-GC struct return buffer can be cleared in the prolog even though its first defining call overwrites it completely. Recognize that call as explicit initialization using the existing return-buffer definition helper and reference counts. Retain initialization for GC-containing buffers, partial definitions, promoted fields, async methods, and potentially observable exception paths.
Split from #133833 at @jakobbotsch's request so this change can be reviewed independently.
Base:
mainata652cdfed564.Focused generated code
Windows x64 method sizes. Tier1 measurements use a repeated harness with tiering and dynamic PGO enabled; the harness waits for tier-up and captures Tier1 output. FullOpts measurements disable tiering for deterministic codegen checks.
FullyDefinedAssembly example:
FullyDefinedFullOpts, Windows x64. Instruction bytes are omitted;
...denotes unchanged assembly.... push rsi push rbx sub rsp, 56 - vxorps xmm4, xmm4, xmm4 - vmovdqa xmmword ptr [rsp+0x20], xmm4 - xor eax, eax - mov qword ptr [rsp+0x30], rax mov rbx, rcx G_M24874_IG02: lea rcx, [rsp+0x20] ... -; Total code size: 112 bytes +; Total code size: 95 bytesSuperPMI results
Windows x64 Checked JIT, eleven SuperPMI collections; identical settings on both sides and loop alignment disabled.
131 contexts shrink; 7 grow by 36 bytes total; 5 equal-size assembly differences. Zero compilation failures. The existing recordings contain 983,424 contexts. Updated main lacks recorded answers in 190 contexts on both sides; those contexts are excluded from the table.
Seven contexts grow by 36 bytes in total through zeroing/layout changes. The two largest increases are 14 bytes each in C# LocalRewriter.MakeDecimalLiteral, with higher static PerfScore. These tradeoffs are included in the net figure.
Validation
.locals initwhile omitting explicit initialization stores. JIT dumps confirm the exception, reference-count and partial-definition guards are exercised. A GC-containing return-buffer test collects before returning; its caller retains GC-safe initialization.GCStress=0xC; the Release test project builds successfully.Release JIT throughput was measured with PIN instruction counts (candidate versus this PR's base):
These small JIT instruction-count differences are not application-throughput measurements. Code size and static PerfScore also do not establish execution speed. No ARM64 or x86 execution is claimed.