JIT: cost fragmented vector-sized struct copies

[benaadams]

Physical promotion can split a vector-sized struct copy into many scalar stores without charging for that extra work. Account for the additional moves when promoting a struct entirely into sub-native fields, preserving vector copies when field promotion is not profitable.

Split from #133833 at @jakobbotsch's request so this change can be reviewed independently.

All comparisons below are from base main at a652cdfed564 to this PR.

Focused generated code

A drawing command supplies an RGBA color as four little-endian floats. Validate the normalized channels, then apply the color to the four vertices of a quad:

using System;

using System.Buffers.Binary;

using System.Runtime.CompilerServices;



public class QuadColor

{

    public readonly record struct Color(float R, float G, float B, float A);



    [MethodImpl(MethodImplOptions.NoInlining)]

    public static Color DecodeColor(ReadOnlySpan<byte> command) => new Color

    {

        R = BinaryPrimitives.ReadSingleLittleEndian(command),

        G = BinaryPrimitives.ReadSingleLittleEndian(command.Slice(4)),

        B = BinaryPrimitives.ReadSingleLittleEndian(command.Slice(8)),

        A = BinaryPrimitives.ReadSingleLittleEndian(command.Slice(12))

    };



    [MethodImpl(MethodImplOptions.NoInlining)]

    public static bool SetQuadColor(ReadOnlySpan<byte> command, Color[] vertexColors)

    {

        Color color = DecodeColor(command);

        // These comparisons also reject NaN and infinities.

        if (!(color.R >= 0 && color.R <= 1 &&

              color.G >= 0 && color.G <= 1 &&

              color.B >= 0 && color.B <= 1 &&

              color.A >= 0 && color.A <= 1))

            return false;



        vertexColors[0] = color;

        vertexColors[1] = color;

        vertexColors[2] = color;

        vertexColors[3] = color;

        return true;

    }

}

The decoder is deliberately kept as a separate call in this measurement. On Windows x64, main promotes the four fields and emits scalar stores for each vertex. This PR preserves vector copies. An excerpt from the FullOpts fast path (unchanged setup and bounds checks omitted):

- lea      rcx, bword ptr [rbx+0x10]

- vmovss   dword ptr [rcx], xmm0

- vmovss   dword ptr [rcx+0x04], xmm1

- vmovss   dword ptr [rcx+0x08], xmm2

- vmovss   dword ptr [rcx+0x0C], xmm3

- lea      rdx, bword ptr [rbx+0x20]

- vmovss   dword ptr [rdx], xmm0

- vmovss   dword ptr [rdx+0x04], xmm1

- vmovss   dword ptr [rdx+0x08], xmm2

- vmovss   dword ptr [rdx+0x0C], xmm3

- lea      r8, bword ptr [rbx+0x30]

- vmovss   dword ptr [r8], xmm0

- vmovss   dword ptr [r8+0x04], xmm1

- vmovss   dword ptr [r8+0x08], xmm2

- vmovss   dword ptr [r8+0x0C], xmm3

- add      rbx, 64

- vmovss   dword ptr [rbx], xmm0

- vmovss   dword ptr [rbx+0x04], xmm1

- vmovss   dword ptr [rbx+0x08], xmm2

- vmovss   dword ptr [rbx+0x0C], xmm3

+ vmovups  xmm0, xmmword ptr [rsp+0x20]

+ vmovups  xmmword ptr [rbx+0x10], xmm0

+ vmovups  xmm0, xmmword ptr [rsp+0x20]

+ vmovups  xmmword ptr [rbx+0x20], xmm0

+ vmovups  xmm0, xmmword ptr [rsp+0x20]

+ vmovups  xmmword ptr [rbx+0x30], xmm0

+ vmovups  xmm0, xmmword ptr [rsp+0x20]

+ vmovups  xmmword ptr [rbx+0x40], xmm0

...

- ; Total method size: 394 bytes

+ ; Total method size: 271 bytes

The bounds-check fallback has the same copy transformation. Scalar-load placement and register assignment also change, and some branches become short encodings. Each vector copy still reloads the source from the stack.

| Windows x64 compilation | Before bytes | After bytes | Reduction | Instructions | Static PerfScore (lower is better) |

| --- | ---: | ---: | ---: | ---: | ---: |

| FullOpts | 394 | 271 | 31.2% | 94 -> 70 | 41.33 -> 33.46 |

| Tier1 with PGO | 329 | 269 | 18.2% | 86 -> 70 | 23.33 -> 22.38 |

Counts cover the whole method, including fallback paths. Tier1 results use the same valid/invalid input sequence on both JITs. Linux x64 FullOpts is unchanged at 573 bytes. Behavioral checks pass on both JITs on Windows and Linux; inputs include a valid color and negative, above-one, NaN and infinite channel values. This is a focused example outside the SuperPMI corpus. Execution speed has not been measured.

Cost accounting

Reuse existing access counters and register-move costs, amortizing the original vector move across fields. The added charge applies when there are more distinct primitive accesses than native-sized copy parts and the participating accesses are smaller than a native word. Count scalar reads and induced accesses without counting the same access twice. Fields with only definitions and no induced accesses remain excluded: promotion can propagate their values into whole-struct copies and eliminate the local. Struct initializations, including nonzero GT_INIT_VAL values, are identified with IsInitVal() and excluded from the existing copy-destination counts; no extra per-access counters are added. Call-result readbacks retain their separate cost.

Whole-local copies whose matching fields are already promoted at the other endpoint receive credit through the existing induced-access pass, for both regular and physical promotion. Partial copies retain their charge, including when the destination has only induced fields. The induced-sibling scan runs only for accesses eligible for this heuristic and when the existing full-copy count is nonzero. This remains a per-field estimate based on access entries, rather than joint evaluation of the final replacement set.

The focused CopyInducedToArray test copies a 16-byte subrange at offset 8 from a 24-byte promoted source, then stores the destination into eight array elements. The destination has no explicit field reads. Main emits scalar array stores; this PR preserves vector copies, reducing the Windows x64 method from 472 to 373 bytes. This example is outside the retained corpus.

SuperPMI

Eleven Windows x64 collections, Checked JITs, identical settings and loop alignment disabled:

| Comparable contexts | Before code bytes | After code bytes | Assembly differences | Regressions |

| ---: | ---: | ---: | ---: | ---: |

| 983,234 | 238,457,491 | 238,457,491 | 0 | 0 |

Zero compilation failures. The recordings contain 983,424 contexts; 190 lack recorded answers needed by both JITs and are excluded. The existing corpus demonstrates no observed generated-code regressions, but does not contain a measured improvement from this PR.

Compilation throughput

Release-JIT PIN instruction counts, main to this PR:

| Collection | Comparable contexts | Before JIT instructions | After JIT instructions | Change |

| --- | ---: | ---: | ---: | ---: |

| benchmarks.run | 51,617 | 87,398,875,380 | 87,425,813,848 | +0.03082% |

| HashSet PGO | 1,307 | 558,877,354 | 558,947,764 | +0.01260% |

Both sides have nonzero counts in every comparable context; HashSet has six shared missing recordings. These counts measure compilation work for the complete PR, not generated-code execution speed. The small measured increases do not justify adding cached per-local state.

Validation

• Windows x64 Checked and Linux x64 Release JIT builds, plus Checked ARM64 and x86 cross-JIT builds.

• SharedVectorCopyCost: Windows normal, forced physical promotion, JitStress=2 and tiering/PGO runs; Linux behavioral and applicable x64 codegen checks.

• Codegen checks cover vector array copies, constant-Guid copies without a stack temporary, and shared copies with register and stack arguments. A four-field case exercises a regularly promoted endpoint, with bare X64 codegen checks on Windows and Linux. Disabling only the regular-endpoint credit makes the check fail on Windows; Linux already chooses that code shape without the credit. The JIT dump confirms regular destination promotion and induced physical source promotion on Windows.

• Test-wrapper MSBuild evaluation confirms exactly one registration on Windows, Android, iOS, Mono browser and CoreCLR browser: conditional child project on supported targets, inline source otherwise. Mobile execution was not run.

• Induced-only partial-copy and mixed-width tests use bare X64 checks and pass on Windows and Linux. The mixed-width case combines a long with four short fields and verifies the no-charge behavior. Removing only the native-width sibling guard causes its Windows codegen check to fail on a stack reload.

• MSBuild evaluation confirms process isolation and explicit DOTNET_JitEnablePhysicalPromotion=1, alongside the existing optimized-code settings.

• Nonzero initblk coverage: the dump confirms GT_INIT_VAL reaches physical promotion and is excluded from the full-struct copy-destination count. Positive/negative inputs and codegen checks cover constant propagation after a partial overwrite.

• Windows-target ReadyToRun codegen validation with matching-source crossgen2 and Checked cross-JITs: ARM64 CopyToArray is 292 -> 248 bytes, and CopyInducedToArray is 448 -> 380 bytes. ARM64 checks require both paired vector stores for the four direct copies and all four paired vector stores for the eight induced copies; both methods reject main at the vector-store checks. On x86 these methods remain 242 and 409 bytes: their int fields are native-sized and intentionally uncharged. New x86 checks cover that exclusion; initialization checks cover both targets. ARM64 and x64 have eight-byte pointers; x86 has four.

• Array-copy checks count all eight induced stores on Windows/Linux x64 and all four direct stores on Windows x64. Checks end at the first return so fallback-path stores cannot satisfy missing fast-path copies, and do not require repeated source loads. SuperFileCheck passes against retained PR listings on Windows/Linux x64 and ARM64; removing each fast-path vector store individually causes all 26 mutation cases to fail. x86 checks also pass.

• Changed-line clang-format, git diff --check, and added-source ASCII checks.

The full JIT test tree has not been rerun. ARM64 and x86 validation compiles and checks generated code; no execution on those targets is claimed. Code size, static PerfScore and JIT instruction counts do not establish generated-code execution speed.

[azure-pipelines]

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.

[dotnet-policy-service]

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

[Copilot]

🔵 Needs a closer look

Resolve the potential double-counting of vector-copy costs and clarify the return-contract documentation.

Pull request overview

This PR improves RyuJIT physical-promotion costing for fragmented vector-sized struct copies.

Changes:

• Tracks store accesses in release builds.
• Accounts for additional register moves when vector copies are fragmented.
• Updates related return-contract documentation.

File summaries

File	Reviewed changes	Findings
src/coreclr/jit/promotion.cpp	Adds vector-copy cost accounting and access tracking.	Moderate (1 vote): A physical vector copy may be charged twice when both locals are promoted. Nit (1 vote): The return-contract comment should describe all FIELD_LIST construction failures.

Review details

Suppressed comments (2)

src/coreclr/jit/promotion.cpp:750

• A single STORE_LCL_VAR dst, src is recorded once as CountStoreSource on src and again as CountStoreDestination on dst. If both locals are promoted into sub-native fields, each side's independent EvaluateReplacement calls charge the same physical vector copy, so a 16-byte copy split into four 4-byte fields is penalized twice (roughly six extra moves instead of the three added by the decomposition). This can reject profitable promotions based on double-counted cost; please deduplicate this per copy or document why the two-sided charge is intentional.

                countVectorCopies += otherAccess.CountStoreSource + otherAccess.CountStoreDestination +
                                     otherAccess.CountPassedAsRetbuf - otherAccess.CountStoredFromCall;
                countVectorCopiesWtd += otherAccess.CountStoreSourceWtd + otherAccess.CountStoreDestinationWtd +
                                        otherAccess.CountPassedAsRetbufWtd - otherAccess.CountStoredFromCallWtd;

src/coreclr/jit/promotion.cpp:2441

• This contract is narrower than the implementation: false is also returned when the argument is not found, the ABI requires a stack/by-reference argument, or the field-list construction rejects a remainder or partial overlap. Please describe the general failure to form a FIELD_LIST rather than attributing every failure to write-backs.

//   True if the argument was replaced; false if write-backs are required.

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Lite

[benaadams]

Addressed the two points from this review in afaadeb.

The cost model now reuses the existing induced-access retry pass. When selected replacements induce matching field accesses at the other endpoint of a whole-local copy, that copy is excluded from the second field's fragmentation charge. One-sided and partial copies retain their cost. This fixes the incremental decision; jointly selecting sets when neither endpoint qualifies initially remains outside the existing algorithm.

Added SharedVectorCopyCost: the previous model promoted only one destination field; the revised model promotes all five, restoring main's codegen. It goes from 340 to 302 bytes, 86 to 81 instructions, and 96 to 80 bytes of local frame space. The new Windows x64 codegen check passes with the fix and fails on the previous model at an unwanted short-field reload. Behavioral tests pass on Windows and Linux, including Windows promotion stress, JitStress=2 and tiering/PGO. The test-project build and 18 existing regression runs also pass.

Replayed 983,234 comparable SuperPMI contexts with no compilation failures (190 shared recording gaps). Corpus totals are unchanged: 14 bytes saved overall, including the two existing 16-byte StringBuilder regressions. The controlled HashSet comparison retains its 1,174-byte saving. Release JIT instruction counts versus main increase 0.00638% on benchmarks and 0.00196% on HashSet; these are compiler-work measurements, not application timings.

Also restored the original FIELD_LIST return-contract comment and updated the PR description with the implementation and validation results.

[Copilot]

🟡 Changes recommended

Unresolved promotion-cost accounting and test-runner gating issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/coreclr/jit/promotion.cpp:888

• The induced-copy credit is not applied when both endpoints already have direct primitive accesses. PickPromotions evaluates those accesses with inducedAccess == nullptr; later InduceAccess returns as soon as the candidate aggregate already overlaps the field, so no CountFullCopies entry is recorded for either endpoint. The same whole-local copy is therefore charged once while evaluating each endpoint, which can reject otherwise profitable promotion. The credit needs to account for matching replacements already present in the other endpoint's aggregate, not only fields newly selected through induced promotion.

            if (inducedAccess != nullptr)
            {
                assert(inducedAccess->CountFullCopies <= countVectorCopies);
                countVectorCopies -= inducedAccess->CountFullCopies;
                countVectorCopiesWtd = max(0.0, countVectorCopiesWtd - inducedAccess->CountFullCopiesWtd);

src/tests/JIT/Directed/Directed_do.csproj:18

• All neighboring physical-promotion projects are gated by _UseOutOfProcessPhysicalPromotionTests, but this new project requires process isolation and is referenced unconditionally. On mobile targets where that property is false, the test is still added to the merged Directed runner even though the other physical-promotion projects are omitted. Apply the same condition here so it is not merged into an unsupported runner.

    <MergedWrapperProjectReference Include="physicalpromotion\SharedVectorCopyCost.csproj" />

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[benaadams]

Added cross-platform regression coverage in c514c29. SharedVectorCopyCost now includes a variant with six integer arguments before the struct, forcing stack passing on Unix x64. Both variants share the same inlined body and use X64 assembly assertions.

The stack-argument check passes with the revised model and fails with the previous cost model on both Windows and Linux. In the final shared-body test, Windows improves from 518 to 478 bytes (115 to 110 instructions), and Linux from 441 to 409 bytes (109 to 101 instructions). Both restore main codegen. Behavioral tests and the Release test-project build pass; the PR description now includes separate Windows and Linux assembly excerpts.

This commit changes tests only. The separate regular-promotion endpoint concern in the latest inline comment is not addressed by this update.

[Copilot]

🔵 Needs a closer look

Two unresolved moderate issues affect whole-copy cost accounting and mobile test integration.

Review details

Suppressed comments (2)

src/coreclr/jit/promotion.cpp:1519

• Only the physical↔physical path sets isFullCopy here. InduceAccessesFromRegularlyPromotedStruct still calls InduceAccess with the default false, so a vector-sized candidate copied to/from an already regularly promoted whole local gets matching induced fields but never subtracts CountFullCopies; its whole-copy fragmentation is charged again even though both endpoints are promoted. Compute/pass the same whole-local predicate in that path, and cover it with a regression case.

[!NOTE] This review comment was created by GitHub Copilot.

            bool isFullCopy = (candOffs == 0) && (inducerOffs == 0) &&
                              (size == m_compiler->lvaGetDesc(candidate)->GetLayout()->GetSize()) &&
                              (size == m_compiler->lvaGetDesc(inducer)->GetLayout()->GetSize());

src/tests/JIT/Directed/Directed_do.csproj:18

• Directed_do.csproj uses _UseOutOfProcessPhysicalPromotionTests to reference the physical-promotion child projects only on supported targets and compiles their .cs sources inline otherwise (lines 84-89). This new reference is unconditional; on mobile targets the referenced project sets DisableProjectBuild=true, but SharedVectorCopyCost.cs has no fallback Compile entry, so the regression test does not participate in that mobile test path. Mirror the existing conditional reference/fallback pattern or explicitly mark this test unsupported on those targets.

[!NOTE] This review comment was created by GitHub Copilot.

    <MergedWrapperProjectReference Include="physicalpromotion\SharedVectorCopyCost.csproj" />

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[benaadams]

Addressed both findings from review 5199750245 in f0b4607:

• Regular-promotion endpoints now pass the whole-copy classification explicitly; the default was removed. Added a four-field test and verified the regular/physical promotion paths in the JIT dump.
• Directed_do.csproj now uses the existing conditional child-project reference and inline-source fallback for SharedVectorCopyCost. MSBuild evaluation confirms exactly one registration on Windows, Android, iOS, Mono browser and CoreCLR browser. Mobile execution was not run.

The cost model also preserves definition-only fields so constant propagation can eliminate their struct storage. The final comparison with main has identical assembly across 983,234 comparable SuperPMI contexts, zero compilation failures and 190 shared missing recordings. The focused readonly Color record example improves Windows FullOpts from 394 to 271 bytes and Tier1/PGO from 329 to 269; Linux FullOpts is unchanged. The PR description now includes its C# and assembly diff, with all results comparing main to the final PR.

Windows optimized/stress/Tier1 tests, Linux checks, and 18 existing regression runs pass. PIN reports small increases in JIT compilation instructions (+0.00805% and +0.00224%); application execution speed has not been measured.

[Copilot]

🔵 Needs a closer look

Definition-only fields may be incorrectly counted, and the regression test lacks a codegen assertion.

Review details

Suppressed comments (2)

src/coreclr/jit/promotion.cpp:742

• primitiveAccessCount includes every primitive entry, even entries where Count == CountStoreDestination and the field is only defined. That makes an otherwise eligible vector copy look fragmented and can reject promotion of the actually-read fields; the comment above says definition-only fields can be propagated and should not contribute to this cost. Exclude definition-only entries from both the count and the mixed-width check (for example, require otherAccess.Count > otherAccess.CountStoreDestination here).

            if (otherAccess.AccessType != TYP_STRUCT)
            {
                primitiveAccessCount++;
                costVectorCopies &= genTypeSize(otherAccess.AccessType) < TARGET_POINTER_SIZE;
            }

src/tests/JIT/Directed/physicalpromotion/SharedVectorCopyCost.cs:130

• This regression only asserts the returned value, so it would still pass if the regular-promotion endpoint were charged again and the optimizer chose the old fragmented code—the isFullCopy accounting changes codegen/cost decisions, not semantics. Please add a disassembly assertion (or another observable promotion check) for this method so reverting the regular-promotion path fix makes the test fail.

    private static int CopyToRegularlyPromotedLocal(int input)
    {
        // The four-field destination is eligible for regular promotion. Whole-local
        // copies must credit that endpoint just as they credit physical promotion.
        ArrayValue source = CreateArrayValue(input);

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[benaadams]

Addressed both findings from review 5200210443 in f9f5ff8.

• Definition-only primitive entries are now excluded from both the fragmentation count and the mixed-width check, using the same existing counters as the target-field filter.
• CopyToRegularlyPromotedLocal now has a bare X64 codegen assertion, validated on Windows and Linux. A counterfactual JIT with only the regular-endpoint credit disabled fails the new assertion on Windows, on the vector stack reload. Linux passes with or without that credit because it already chooses the checked shape there.

The eleven-collection SuperPMI replay has identical assembly across 983,234 comparable contexts, zero compilation failures and 190 shared missing recordings. Windows optimized, forced-promotion, JitStress=2 and Tier1/PGO tests pass; Linux behavioral and codegen checks pass. The Color example remains 271 bytes FullOpts and 269 bytes Tier1 on Windows.

The description has been updated with the final cost-accounting rules, cross-platform assertion coverage and refreshed PIN measurements against main. The separate load-reuse experiment is not included.

[Copilot]

🔵 Needs a closer look

The JIT cost-accounting changes warrant final human review.

Review details

Suppressed comments (1)

src/coreclr/jit/promotion.cpp:1486

• The identical isFullCopy predicate is maintained separately in this function and in InduceAccessesInCandidate below. Keeping the whole-local classification duplicated makes these two induction paths easy to diverge; factor it into one helper and use that helper at both call sites so the full-copy accounting cannot regress in only one path.

        bool isFullCopy = (candidateOffs == 0) && (regPromOffs == 0) &&
                          (size == m_compiler->lvaGetDesc(candidateLcl)->GetLayout()->GetSize()) &&
                          (size == regPromDsc->GetLayout()->GetSize());

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[benaadams]

Addressed the duplicate whole-copy predicate from the review in 3ea3712. Both induction paths now use one helper, which reuses Compiler::IsEntireAccess for each endpoint and preserves partial-copy handling.

The isolated Windows x64 Checked build and focused behavior/codegen checks pass, including forced promotion, JitStress=2 and Tier1/PGO. A three-collection SuperPMI comparison against the previous PR head has identical assembly across 87,586 matched contexts, no compilation failures and six shared recording misses. The PR's generated-code example is unaffected. Linux and the full suite were not rerun for this refactor.

[Copilot]

🔵 Needs a closer look

Moderate findings remain in the promotion heuristic and test configuration.

Review details

Suppressed comments (4)

src/coreclr/jit/promotion.cpp:895

• This is a global promotion-decision heuristic, but the validation reports only code size, static PerfScore, and JIT-work deltas; it explicitly does not measure generated-program throughput. Because this charge can suppress field promotion for all 16-byte SIMD-layout structs, please add a representative execution benchmark or document why code-size evidence is sufficient for this tradeoff before relying on it as a performance improvement.

            costWith += countVectorCopiesWtd * extraMoves * COST_REG_ACCESS_CYCLES;

src/coreclr/jit/promotion.cpp:741

• The mixed-width gate is an explicit part of this heuristic, but the added disassembly tests exercise only 4-byte and 2-byte fields on x64, so every tested primitive access remains smaller than the native word. A regression that incorrectly charges (or fails to charge) a 16-byte struct containing an 8-byte field would pass these tests; add a codegen case with a native-width access alongside smaller fields and assert the intended no-charge behavior.

                costVectorCopies &= genTypeSize(otherAccess.AccessType) < TARGET_POINTER_SIZE;

src/coreclr/jit/promotion.cpp:730

• EvaluateReplacement is also called for induced-only fields, where PickInducedPromotions passes a zero-count fakeAccess. In that case this gate is false (0 > 0), and the primitive count below scans only m_accesses, so induced-only sub-native fields can never incur the new fragmentation cost. Since CountFullCopies is only subtracted after this gate, partial whole-struct copies are also left uncharged; include induced accesses in the eligibility/count calculation while retaining the full-copy credit.

        bool costVectorCopies = (access.Count > access.CountStoreDestination) &&
                                (genTypeSize(access.AccessType) < TARGET_POINTER_SIZE) &&
                                varTypeIsSIMD(layout->GetRegisterType());

src/tests/JIT/Directed/physicalpromotion/SharedVectorCopyCost.csproj:12

• This disassembly test relies on physical promotion being enabled, but the project does not set DOTNET_JitEnablePhysicalPromotion=1. A run under a configuration or test scenario that sets this switch to 0 will skip PhysicalPromotion entirely, so the checks no longer exercise this regression (and can fail on the expected promoted-code patterns). The sibling physical-promotion projects set this variable explicitly (for example, physicalpromotion/fuzzlyn1.csproj:10); add the same environment entry here.

    <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
    <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[benaadams]

Addressed the three code/test points from this review in 8f8a736:

• The test project explicitly sets DOTNET_JitEnablePhysicalPromotion=1; MSBuild evaluation confirms it and process isolation.
• Added a mixed-width case with a long and four short fields. Its bare X64 checks pass on Windows and Linux. Removing only the native-width sibling guard makes the Windows check fail on a stack reload, so the check detects the intended no-charge behavior.
• Induced accesses now participate in eligibility and field counting without double-counting explicit scalar reads. Full-copy credit is retained. A partial-copy test has a 16-byte destination with no explicit field reads, sourced at offset 8 of a 24-byte promoted local. Preserving its vector array copies reduces the method from 472 to 373 bytes on Windows and 503 to 367 on Linux. The codegen-check suite fails on the prior JIT on both platforms.

Struct initializations are excluded from the existing destination counters used for copy costing. This avoids charging initialization as a copy and adds no per-access fields. It also preserves the console formatter's baseline assembly in the retained corpus.

Windows Checked and Linux Release builds, focused behavior and codegen checks pass. Windows coverage includes forced promotion, JitStress=2 and Tier1/PGO. All eleven SuperPMI collections retain identical assembly across 983,234 matched contexts, with no compilation failures or new misses and 190 shared recording misses.

The description now reflects the net cost-accounting behavior, focused coverage and current validation limits.

[Copilot]

🟡 Changes recommended

Unresolved moderate findings remain around initialization handling and compilation-time cost, with target-specific coverage gaps.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/coreclr/jit/promotion.cpp:1633

• This recognizes only integral-constant zero initialization, but the JIT also represents initblk patterns with GT_INIT_VAL; GenTree::IsInitVal() includes both forms, and gtNewStoreValueNode can turn such a value into a GT_STORE_LCL_VAR. Those stores are therefore still counted as CountStoreDestination and can be charged as vector copies, contrary to the stated exclusion for struct initialization. Use IsInitVal() here and cover a nonzero initblk case.

            if (lcl->TypeIs(TYP_STRUCT) && lcl->Data()->gtEffectiveVal()->IsIntegralConst())
            {
                flags |= AccessKindFlags::IsInit;

src/coreclr/jit/promotion.cpp:905

• This branch uses TARGET_POINTER_SIZE to change the promotion threshold, so a 16-byte SIMD copy is charged differently on x86/ARM64 than on x64. The new codegen assertions cover only X64 (and X64-WINDOWS), while the behavioral assertions cannot detect a target-specific promotion/costing regression. Please add at least an ARM64/x86 codegen check or explain why those targets are intentionally out of scope.

[!NOTE] This review comment was created by GitHub Copilot.

        unsigned nativeParts = layout->GetSize() / TARGET_POINTER_SIZE;
        if (costVectorCopies && (primitiveAccessCount > nativeParts))
        {

src/tests/JIT/Directed/physicalpromotion/SharedVectorCopyCost.cs:174

• The implementation is not x64-only: ClassLayout::GetRegisterType() treats 16-byte non-GC layouts as TYP_SIMD16 on every FEATURE_SIMD target, and the threshold changes with TARGET_POINTER_SIZE. These are the only disassembly assertions in the new suite, so ARM64 and x86 executions exercise behavior without detecting a target-specific promotion or copy-shape regression. Add applicable target-specific codegen coverage (or otherwise validate the heuristic on those supported SIMD targets).

        // X64-WINDOWS: call {{.*}}SharedVectorCopyCost:CheckFields
        // X64-WINDOWS: call {{.*}}SharedVectorCopyCost:CheckFields
        // X64-WINDOWS: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[benaadams]

Addressed the initializer and target-coverage points from this review in c1340aa.

• Initialization now uses IsInitVal(). The new nonzero initblk test reaches physical promotion with GT_INIT_VAL; its dump confirms the full-struct access is excluded from CountStoreDestination. Windows and Linux x64 behavior and codegen checks pass.
• Added ARM64 vector-copy and x86 native-field-exclusion codegen checks, validated with matching-source crossgen2 and Checked cross-JITs. For Windows-target ReadyToRun, ARM64 CopyInducedToArray improves from 448 to 380 bytes versus main; its check fails on main at the expected vector load. x86 remains at 409 bytes because its int fields are native-sized and intentionally uncharged. Initialization checks also pass on both targets. These are cross-compilation/codegen checks, not ARM64 or x86 execution claims.
• One correction to the target-size observation: ARM64 and x64 both use eight-byte pointers; x86 uses four. The heuristic remains available on supported SIMD targets.

The description includes the target coverage and current Release-JIT instruction-count measurements. I replied separately to the scan-cost thread with those measurements.

[Copilot]

🔵 Needs a closer look

Moderate codegen-test coverage gaps remain and require review before approval.

Review details

Suppressed comments (3)

src/tests/JIT/Directed/physicalpromotion/SharedVectorCopyCost.cs:229

• This method performs eight destination[i] = value copies, but the disassembly check requires only two vector load/store pairs. Because SuperFileCheck scopes these ordered checks to this method, a regression that scalarizes six of the eight array stores can still pass. Add checks for all eight vector stores (or an equivalent assertion that every assignment remains a vector copy).

        // X64: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64: {{v?movups}} xmmword ptr
        // X64: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64: {{v?movups}} xmmword ptr

src/tests/JIT/Directed/physicalpromotion/SharedVectorCopyCost.cs:217

• The ARM64 check likewise requires only one vector load and one vector store even though this method writes eight array elements. SuperFileCheck limits the match to CopyInducedToArray, so it does not prove that the remaining seven stores were not scalarized; add enough target-appropriate checks to cover all eight assignments.

        // ARM64: ldr q{{[0-9]+}}, [{{fp|sp}}
        // ARM64: {{stp|str}} q{{[0-9]+}},

src/tests/JIT/Directed/physicalpromotion/SharedVectorCopyCost.cs:174

• The direct full-copy case has disassembly assertions only under X64-WINDOWS, so ARM64 can regress from vector array stores to scalar stores while the behavioral checks still pass. The new cost path is applicable to ARM64 as well; add ARM64-specific checks for CopyToArray (or a separate ARM64 codegen test).

        // X64-WINDOWS: call {{.*}}SharedVectorCopyCost:CheckFields
        // X64-WINDOWS: call {{.*}}SharedVectorCopyCost:CheckFields
        // X64-WINDOWS: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmm{{[0-9]+}}, xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[benaadams]

Addressed the three coverage points in 36ea665. CopyInducedToArray now checks all eight x64 vector stores and all four ARM64 paired vector stores. CopyToArray also checks both ARM64 paired stores. The checks are bounded by the first return, so stores in the fallback path cannot hide missing fast-path copies, and they do not require repeated source loads or specific registers.

SuperFileCheck passes against the retained Windows/Linux x64 and ARM64 PR assembly, with x86 checks also passing. Removing each fast-path vector store individually makes all 26 mutation cases fail; ARM64 main fails the vector-store checks. This is test-directive-only validation against retained assembly, not a new ARM64 execution run. The PR description now reflects this coverage.

[Copilot]

🔵 Needs a closer look

Address the two moderate test assertion and X86 coverage issues.

Review details

Suppressed comments (2)

src/tests/JIT/Directed/physicalpromotion/SharedVectorCopyCost.cs:177

• The direct four-int ArrayValue path has no X86 disassembly assertion. Since this heuristic deliberately depends on TARGET_POINTER_SIZE, x86 is the platform where these four native-sized fields must remain uncharged; the X86 check later in CopyInducedToArray covers only the induced-access path. Add an X86 no-vector check here so a regression in the regular-access path is caught as well.

        // X64-WINDOWS: call {{.*}}SharedVectorCopyCost:CheckFields
        // X64-WINDOWS: call {{.*}}SharedVectorCopyCost:CheckFields
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS-LABEL: {{^ *ret}}

src/tests/JIT/Directed/physicalpromotion/SharedVectorCopyCost.cs:176

• These patterns match loads and stores alike: x64 prints both as vmovups xmmword ptr .... The method can therefore satisfy these four checks with source reloads while still emitting scalar stores to the array, so the check would not catch the regression this test is intended to guard. Match the store operand (for example, xmmword ptr [...], xmmN) or otherwise anchor each check to the destination store.

        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr
        // X64-WINDOWS: {{v?movups}} xmmword ptr

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite