JIT: Unify LowerBlockStore for init across targets#128683
Open
EgorBo wants to merge 4 commits into
Open
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
EgorBo
force-pushed
the
unify-lower-init-blkstore
branch
from
4e02781 to
a45ce7b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates init-block (memset) lowering across targets by moving per-target LowerBlockStore init-only logic into a shared LowerInitBlockStore implementation in lower.cpp, while removing the now-dead xarch rep movs/stos block-store path and associated stress mode plumbing.
Changes:
- Introduces shared
LowerInitBlockStoreand routes initGT_STORE_BLKlowering through it. - Adds
FEATURE_HAS_ZERO_REGper-target to enable shared code to choose a contained zero source where supported. - Deletes the xarch
BlkOpKindRepInstrblock-store kind and its codegen/LSRA handling, and removesSTORE_BLOCK_UNROLLINGstress mode.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/targetx86.h | Defines FEATURE_HAS_ZERO_REG (0) for x86. |
| src/coreclr/jit/targetwasm.h | Defines FEATURE_HAS_ZERO_REG (0) for wasm. |
| src/coreclr/jit/targetriscv64.h | Defines FEATURE_HAS_ZERO_REG (1) for riscv64. |
| src/coreclr/jit/targetloongarch64.h | Defines FEATURE_HAS_ZERO_REG (1) for loongarch64. |
| src/coreclr/jit/targetarm64.h | Defines FEATURE_HAS_ZERO_REG (1) for arm64. |
| src/coreclr/jit/targetarm.h | Defines FEATURE_HAS_ZERO_REG (0) for arm32. |
| src/coreclr/jit/targetamd64.h | Defines FEATURE_HAS_ZERO_REG (0) for amd64. |
| src/coreclr/jit/lsraxarch.cpp | Removes LSRA handling tied to the deleted BlkOpKindRepInstr block-store kind. |
| src/coreclr/jit/lsrabuild.cpp | Removes kill-set logic specific to BlkOpKindRepInstr. |
| src/coreclr/jit/lowerxarch.cpp | Deletes xarch-specific initblk LowerBlockStore implementation (now shared). |
| src/coreclr/jit/lowerwasm.cpp | Deletes wasm-specific initblk LowerBlockStore implementation (now shared). |
| src/coreclr/jit/lowerriscv64.cpp | Deletes riscv64-specific initblk LowerBlockStore implementation (now shared). |
| src/coreclr/jit/lowerloongarch64.cpp | Deletes loongarch64-specific initblk LowerBlockStore implementation (now shared). |
| src/coreclr/jit/lowerarmarch.cpp | Deletes arm/arm64-specific initblk LowerBlockStore implementation (now shared). |
| src/coreclr/jit/lower.h | Renames the lowering entrypoint from LowerBlockStore to LowerInitBlockStore for initblk. |
| src/coreclr/jit/lower.cpp | Adds shared LowerInitBlockStore and wires initblk dispatch to it. |
| src/coreclr/jit/gentree.h | Removes GenTreeBlk::BlkOpKindRepInstr from the block-store kind enum. |
| src/coreclr/jit/gentree.cpp | Removes tree-dump formatting for the deleted BlkOpKindRepInstr kind. |
| src/coreclr/jit/compiler.h | Removes STRESS_MODE(STORE_BLOCK_UNROLLING) from the stress mode list. |
| src/coreclr/jit/codegenxarch.cpp | Removes xarch codegen dispatch and helpers for rep movs/stos block-store kinds. |
| src/coreclr/jit/codegen.h | Removes declarations for the deleted xarch rep movs/stos codegen helpers. |
Hoist the per-target `Lowering::LowerBlockStore` (which only handles
InitBlk after the recent `LowerCopyBlockStore` extraction) into a shared
`Lowering::LowerInitBlockStore` in `lower.cpp`, mirroring the
`LowerCopyBlockStore` pattern.
- Rename `LowerBlockStore` -> `LowerInitBlockStore` everywhere.
- Introduce `FEATURE_HAS_ZERO_REG` in each target.h (1 for arm64,
loongarch64, riscv64; 0 for amd64, x86, arm, wasm) to replace
`#if defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || ...`
in the shared lowering.
- Remove `BlkOpKindRepInstr` and all related dead code:
- `genCodeForInitBlkRepStos` / `genCodeForCpBlkRepMovs` and the
`genCodeForStoreBlk` dispatch case
- LSRA: kill-set case and the two `BuildBlockStore` cases
- `gentree.h` enum value and `gentree.cpp` print
- x86 now takes exactly the same path as x64: previously the
`too-big-to-unroll` and `not-CNS-INT` init blk cases used
`rep stosb`; both now go through `LowerBlockStoreAsHelperCall`
(matches the existing TODO-X86-CQ comment).
- Remove the `STRESS_STORE_BLOCK_UNROLLING` stress mode (no value).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
EgorBo
force-pushed
the
unify-lower-init-blkstore
branch
from
a45ce7b to
5a4a5c4
Compare
This was referenced May 28, 2026
Open
Address Copilot PR review feedback: 'isCopyBlk' was unused after removing the BlkOpKindRepInstr case, and the remaining switch only had no-op cases. All current block-store kinds (Unroll/UnrollMemmove/Loop/Invalid) leave the kill set as RBM_NONE, so collapse the function body to a direct return and document why no kill set is needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
PTAL @jakobbotsch @dotnet/jit-contrib clean up for block inits. The shared version reduced copy-paste. Still had to use a couple of ifdefs inside it but I think we might hide some of them via some "Target->IsX" properties in a nice way |
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.
Reduce copy-paste in LowerBlockStore for init-blocks via just one shared impl. Probably a few ifdefs can be removed if we hide them under some "Target's properties". Except for WASM since its impl is completely different.
Also, remove
BlkOpKindRepInstrentirely - it causes some diffs on x86 where we now either unroll or call memset like on other targets, but it should be for better performance.Diffs (non-zero for x86 due to BlkOpKindRepInstr removal)