Skip to content

Remove more write-barriers #128238

Open
EgorBo wants to merge 10 commits into
mainfrom
improve-wb
Open

Remove more write-barriers #128238
EgorBo wants to merge 10 commits into
mainfrom
improve-wb

Conversation

@EgorBo
Copy link
Copy Markdown
Member

@EgorBo EgorBo commented May 15, 2026

  1. GT_STORE_BLK now uses the same address analysis as GT_STOREIND

EgorBo and others added 2 commits May 15, 2026 01:44
For block stores that contain GC pointers, attempt to prove via VN/assertion
analysis that the destination address is not on the GC heap, and if so set
GTF_IND_TGT_NOT_HEAP. This enables fast non-barrier codegen for CpObj
(genCodeForCpObj's dstOnStack path replacing per-slot CORINFO_HELP_ASSIGN_BYREF
with movsp) and SIMD-based zeroing for InitBlk (via IsOnHeapAndContainsReferences).

Also update IsOnHeapAndContainsReferences() to honor GTF_IND_TGT_NOT_HEAP.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 15, 2026 00:17
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 15, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Extends the JIT's write-barrier elimination (previously only for STOREIND) to STORE_BLK nodes. When VN/assertion analysis can prove that a block-store destination is not on the GC heap, the new helper sets GTF_IND_TGT_NOT_HEAP, which lets codegen take the fast non-barrier CpObj/SIMD InitBlk paths. Also strengthens GetWriteBarrierForm's GT_ADD analysis so it can recurse on the TYP_BYREF/TYP_REF operand even when the offset is non-constant, mirroring GCInfo::gcWriteBarrierFormFromTargetAddress.

Changes:

  • Add optWriteBarrierAssertionProp_StoreBlk and dispatch it from optAssertionProp_BlockStore.
  • Teach GetWriteBarrierForm's GT_ADD case to identify the BYREF/REF base operand by VN type when neither operand is a constant; switch the func compare to VNF_ADD and route the StoreInd calls through optConservativeNormalVN (also normalizes the VN before VNVisitReachingVNs).
  • Update GenTreeBlk::IsOnHeapAndContainsReferences to honor GTF_IND_TGT_NOT_HEAP.

Reviewed changes

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

File Description
src/coreclr/jit/gentree.h IsOnHeapAndContainsReferences now also returns false when GTF_IND_TGT_NOT_HEAP is set, so the new assertion-prop result feeds CpObj/InitBlk decisions.
src/coreclr/jit/compiler.h Declares the new optWriteBarrierAssertionProp_StoreBlk; widens optConservativeNormalVN to take const GenTree*.
src/coreclr/jit/assertionprop.cpp Adds the BYREF-base recursion in GetWriteBarrierForm, switches to VNF_ADD, normalizes VNs through optConservativeNormalVN, and implements the new STORE_BLK write-barrier removal helper.

Comment thread src/coreclr/jit/assertionprop.cpp Outdated
@github-actions

This comment has been minimized.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 15, 2026 00:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings May 15, 2026 00:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/coreclr/jit/assertionprop.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 15, 2026 00:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/coreclr/jit/assertionprop.cpp
@github-actions

This comment has been minimized.

@EgorBo EgorBo marked this pull request as ready for review May 15, 2026 02:14
@github-actions

This comment has been minimized.

@EgorBo
Copy link
Copy Markdown
Member Author

EgorBo commented May 15, 2026

@MihuBot

@EgorBo
Copy link
Copy Markdown
Member Author

EgorBo commented May 15, 2026

@MihuBot -nuget

@MihaZupan
Copy link
Copy Markdown
Member

@MihuBot -nuget

Copilot AI review requested due to automatic review settings May 15, 2026 15:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings May 16, 2026 16:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

Caution

Security scanning requires review for Code Review

Details

The threat detection results could not be parsed. The workflow output should be reviewed before merging.

Review the workflow run logs for details.

Note

This review was generated by Copilot.

🤖 Copilot Code Review — PR #128238

Holistic Assessment

Motivation: Justified. GT_STORE_BLK nodes containing GC pointers already benefit from write-barrier elimination when the address is a GT_LCL_ADDR, but CSE can hide that behind a local, making this optimization unreachable. Extending the same VN-based analysis from STOREIND to STORE_BLK is a natural next step.

Approach: Sound. The new optWriteBarrierAssertionProp_StoreBlk mirrors the address-analysis portion of the existing STOREIND path, and the IsOnHeapAndContainsReferences change propagates the flag so downstream lowering/codegen respects it. The const qualification and VNF_ADD fix are good cleanups.

Summary: ✅ LGTM. The change is small, consistent with established patterns, and the previous review concerns (type mismatch bug, missing WBF_BarrierUnchecked handling) have been addressed in subsequent commits.


Detailed Findings

✅ Correctness — Address analysis reuse is safe

GetWriteBarrierForm is a pure function over VNs and is already validated by the STOREIND path. Reusing it for STORE_BLK addresses is correct. The early-out conditions (local assertion prop, no GC pointers, flag already set) match the STOREIND guard logic.

IsOnHeapAndContainsReferences change is consistent

Adding (gtFlags & GTF_IND_TGT_NOT_HEAP) == 0 means callers in lowering and codegen (lowerxarch, codegenxarch, lsraxarch) that use this predicate to decide whether SIMD moves are safe will now correctly account for the new assertion-derived flag — a block proven not-on-heap won't be forced into per-pointer stores.

💡 JITDUMP messages end with trailing ": "

The JITDUMP strings in the new function ("Add GTF_IND_TGT_NOT_HEAP to STORE_BLK [%06d]: ") end with a colon-space but nothing follows before the return. The STOREIND counterpart prints a descriptive suffix ("is not needed at all.\n"). This is cosmetic and doesn't affect correctness, but adding a \n or a brief suffix would improve diagnostic readability.

✅ VNF_ADD fix and const qualification

The VNFunc(GT_ADD)VNF_ADD change aligns with how VN funcs are referenced elsewhere in the file (e.g., line 5440). The const GenTree* change to optConservativeNormalVN is safe since the function only reads gtVNPair.

Generated by Code Review for issue #128238 · ● 1.9M ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants