JIT: STORE_LCL_FLD coalescing by AndyAyersMS · Pull Request #126562 · dotnet/runtime

AndyAyersMS · 2026-04-05T15:46:05Z

Refactor the lowering-time store coalescing machinery so adjacent constant GT_STORE_LCL_FLD writes can reuse the existing combine logic that was previously limited to STOREIND/STORE_BLK. Gate the new local-field path from LowerStoreLocCommon with JitEnableStoreLclFldCoalescing. Extend the struct-promotion regression coverage for the newly handled cases.

Refactor the lowering-time store coalescing machinery so adjacent constant GT_STORE_LCL_FLD writes can reuse the existing combine logic that was previously limited to STOREIND/STORE_BLK. Gate the new local-field path from LowerStoreLocCommon with JitEnableStoreLclFldCoalescing. Extend the struct-promotion regression coverage for the newly handled cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

AndyAyersMS · 2026-04-05T15:47:00Z

@EgorBo PTAL
fyi @dotnet/jit-contrib

Supersedes #126178

dotnet-policy-service · 2026-04-05T15:47:21Z

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

Copilot

Pull request overview

This PR refactors the JIT lowering-time store coalescing logic to extend existing adjacent-constant-store combining (previously focused on indirections) to also handle adjacent GT_STORE_LCL_FLD writes, guarded by a new JIT config switch, and adds regression coverage for struct-promotion-related cases.

Changes:

Extend store coalescing to support GT_STORE_LCL_FLD (including some overlapping-store scenarios) behind JitEnableStoreLclFldCoalescing.
Refactor coalescing machinery to share logic across store kinds and centralize atomicity checks.
Add JIT regression tests covering non-address-exposed, address-exposed, and overlapping local-field store patterns.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/tests/JIT/Directed/StructPromote/SpAddr.cs	Adds new regression scenarios for local-field store coalescing, including overlap and address-exposed cases.
src/coreclr/jit/lower.h	Extends coalescing data structure and declares new shared coalescing/atomicity helpers.
src/coreclr/jit/lower.cpp	Implements shared coalescing path for `STORE_LCL_FLD` and refactors existing store-indir coalescing to reuse it.
src/coreclr/jit/jitconfigvalues.h	Adds `JitEnableStoreLclFldCoalescing` release config gate (default enabled).

Copilot · 2026-04-05T15:54:56Z

src/coreclr/jit/lower.cpp

+                                                 GenTree*                       prevStore,
+                                                 const LoadStoreCoalescingData& currData,
+                                                 const LoadStoreCoalescingData& prevData)
+{


LowerCheckCoalescedStoreAtomicity takes a prevStore parameter but never uses it. This can trigger unused-parameter warnings (depending on toolchain) and makes the API misleading. Either drop the parameter or explicitly mark it unused (e.g., (void)prevStore; / UNUSED(prevStore)) or use it in an assert/JITDUMP to justify its presence.

Suggested change

{

{

(void)prevStore;

kg · 2026-04-07T00:47:06Z

src/coreclr/jit/lower.cpp

+    auto isNodeInvariant = [](Compiler* compiler, GenTree* node) {
+        if (node->OperIsConst())
+        {
+            return true;
+        }
+
+        return node->OperIs(GT_LCL_VAR) && !compiler->lvaVarAddrExposed(node->AsLclVar()->GetLclNum());
+    };
+
+    auto* lclStore = store->AsLclFld();
+    if (!isNodeInvariant(m_compiler, lclStore->Data()))
+    {
+        return false;
+    }


Am I missing something or is this lambda only used once?

I assume it was copied from GetLoadStoreCoalescingData, it probably needs to be a separate static function then

kg · 2026-04-07T01:04:12Z

src/coreclr/jit/lower.cpp

+#ifdef TARGET_ARM64
+        if (currData.accessSize == TARGET_POINTER_SIZE)
+        {
+            // Per Arm ARM, a 128-bit SIMD write that is 64-bit aligned is treated as a pair of


A citation URL or something here would be cool

src/coreclr/jit/lower.cpp

kg · 2026-04-07T01:17:37Z

src/coreclr/jit/lower.cpp

-        // items at once. Although, it may be profitable to do "stp q0, q0, [addr]".
-        if (!varTypeIsIntegral(ind) && !varTypeIsSIMD(ind))
+        if (!varTypeIsIntegral(node) && !varTypeIsSIMD(node))
        {


A lot of comments were removed in this method and it seems like some of them might have been worth keeping, just want to double-check that they weren't blanket removed accidentally, like this CQ one

kg · 2026-04-07T01:19:07Z

src/coreclr/jit/lower.cpp

-        // it on ARM64 where it's pretty efficient to do "stp xzr, xzr, [addr]" to clear two
-        // items at once. Although, it may be profitable to do "stp q0, q0, [addr]".
-        if (!varTypeIsIntegral(ind) && !varTypeIsSIMD(ind))
+        if (!varTypeIsIntegral(node) && !varTypeIsSIMD(node))


I wonder why we don't support coalescing for adjacent floats?

I don't remember why it wasn't done, but it was like that before, so could be a task for a follow-up

I would expect both float and SIMD to be supportable if integers are supported.

It's not up to this PR anyway. I think we already lower *x = 3.14 into TYP_INT store today before we get here.

Sure, was just noting in agreement that this seems like an artificial limitation we can reduce in the future.

kg · 2026-04-07T01:20:09Z

src/coreclr/jit/lower.cpp

+                case 4:
+                    newType = TYP_INT;
+                    break;
+#ifdef TARGET_64BIT


or WASM (eventually)

kg · 2026-04-07T01:20:41Z

src/coreclr/jit/lower.cpp

 {
-// LA, RISC-V and ARM32 more likely to receive a terrible performance hit from
-// unaligned accesses making this optimization questionable.
 #if defined(TARGET_XARCH) || defined(TARGET_ARM64)


Do we want to enable this for wasm while we're here, or add a todo-wasm comment to remind ourselves to come back later and turn it on?

kg · 2026-04-07T01:23:43Z

src/coreclr/jit/lower.cpp

        {
-            // RetBuf is a private stack memory, so we don't need to worry about atomicity.
-            allowsNonAtomic = true;
+            BlockRange().Remove(prevData.rangeStart, prevData.rangeEnd);


Maybe add a JITDUMP message when doing this so it's clear what happened?

Yeah I think it's worth a comment or JitDump that in this case we remove a store because the next stores overwrites it.

kg · 2026-04-07T01:25:40Z

src/coreclr/jit/lower.cpp

+                    newType = TYP_LONG;
+                    break;

 #if defined(FEATURE_HW_INTRINSICS)


Aren't there targets that have FEATURE_HW_INTRINSICS but not FEATURE_SIMD? Should this be gated on SIMD instead?

in theory. I think it's indeed should be FEATURE_SIMD here

FEATURE_HW_INTRINSICS is built on top of FEATURE_SIMD (unless one of the community driven platforms has introduced a deviation).

It's something that ideally we'd merge together into a single feature, particularly given how co-dependent they are now for practical usage.

kg · 2026-04-07T01:26:19Z

src/coreclr/jit/lower.cpp

+#endif
+#endif
+#endif


These would benefit from comments specifying what if they're attached to

kg · 2026-04-07T01:29:21Z

src/coreclr/jit/lower.cpp

+                    newType = TYP_INT;
+                    break;

 #ifdef TARGET_64BIT


kg · 2026-04-07T01:46:55Z

src/coreclr/jit/lower.cpp

+            if (prevData.offset > currData.offset)
+            {
+                std::swap(lowerCns, upperCns);
+            }
+


Why was this swap moved from outside of here to inside of 64bit+hwintrins+simd? Does it fix a bug?

kg · 2026-04-07T01:57:55Z

src/coreclr/jit/lower.cpp

-
    } while (true);
-#endif // TARGET_XARCH || TARGET_ARM64
+#endif


Endif could use a comment

EgorBo · 2026-04-07T13:38:18Z

src/coreclr/jit/lower.cpp

    }

    data->targetType = ind->TypeGet();
+    data->accessSize = ind->Size();


if we add size, we probably no longer need targetType field

EgorBo · 2026-04-07T14:01:00Z

src/coreclr/jit/lower.cpp

-        if (prevData.offset == currData.offset)
+        if ((currData.offset == prevData.offset) && (currData.targetType == prevData.targetType))
        {
+            if (m_compiler->gtTreeHasSideEffects(prevData.value, GTF_SIDE_EFFECT | GTF_GLOB_EFFECT | GTF_ASG) ||


I'm confused by GTF_SIDE_EFFECT | GTF_GLOB_EFFECT | GTF_ASG - doesn't GTF_GLOB_EFFECT imply them all alone?

EgorBo · 2026-04-07T14:11:37Z

src/coreclr/jit/lower.cpp

-                    //
-                    // but we don't want to load managed references into SIMD registers (we can only do so
-                    // when we can issue a nongc region for a block)
-                    return;


is there a reason this comment was removed? I think it explains why we bail out here

EgorBo · 2026-04-07T14:13:17Z

src/coreclr/jit/lower.cpp

            int8_t* upperCns = currData.value->AsVecCon()->gtSimdVal.i8;

-            // if the previous store was at a higher address, swap the constants
            if (prevData.offset > currData.offset)


the comment explained why we guarded #if defined(TARGET_AMD64) - i think we should keep it.

EgorBo · 2026-04-07T14:28:25Z

I think we need to look at CI results before the next round of reviews

Copilot AI review requested due to automatic review settings April 5, 2026 15:46

github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 5, 2026

dotnet-policy-service bot assigned AndyAyersMS Apr 5, 2026

Copilot started reviewing on behalf of AndyAyersMS April 5, 2026 15:47 View session

AndyAyersMS mentioned this pull request Apr 5, 2026

Store local field coalesce #126178

Closed

Copilot AI reviewed Apr 5, 2026

View reviewed changes

kg reviewed Apr 7, 2026

View reviewed changes

src/coreclr/jit/lower.cpp Show resolved Hide resolved

kg reviewed Apr 7, 2026

View reviewed changes

src/coreclr/jit/lower.cpp

case 4:

newType = TYP_INT;

break;

#ifdef TARGET_64BIT

Copy link
Copy Markdown

Member

kg Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or WASM (eventually)

kg reviewed Apr 7, 2026

View reviewed changes

src/coreclr/jit/lower.cpp

newType = TYP_INT;

break;

#ifdef TARGET_64BIT

Copy link
Copy Markdown

Member

kg Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or WASM

kg reviewed Apr 7, 2026

View reviewed changes

src/coreclr/jit/lower.cpp

} while (true);

#endif // TARGET_XARCH || TARGET_ARM64

#endif

Copy link
Copy Markdown

Member

kg Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Endif could use a comment

kg approved these changes Apr 7, 2026

View reviewed changes

EgorBo reviewed Apr 7, 2026

View reviewed changes

AndyAyersMS closed this Apr 8, 2026

AndyAyersMS reopened this Apr 8, 2026

build-analysis bot mentioned this pull request Apr 8, 2026

System.Net.NameResolution.Tests DNS failures: Name or service not known #126641

Open

Conversation

AndyAyersMS commented Apr 5, 2026

Uh oh!

AndyAyersMS commented Apr 5, 2026

Uh oh!

dotnet-policy-service bot commented Apr 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

EgorBo Apr 7, 2026 •

edited

Loading

EgorBo Apr 7, 2026 •

edited

Loading

EgorBo Apr 7, 2026 •

edited

Loading