Skip to content

Arm64: Initialization of TYP_SIMD/TYP_MASK locals - #128148

Merged
jakobbotsch merged 10 commits into
dotnet:mainfrom
snickolls-arm:local-initialization
Jul 28, 2026
Merged

Arm64: Initialization of TYP_SIMD/TYP_MASK locals#128148
jakobbotsch merged 10 commits into
dotnet:mainfrom
snickolls-arm:local-initialization

Conversation

@snickolls-arm

Copy link
Copy Markdown
Contributor

TYP_SIMD/TYP_MASK locals are zero-initialized by writing the entire UnknownSizeFrame through a short loop of vector stores.

TYP_SIMD locals are poisoned individually for debugging code, by broadcasting the poison value to a vector and storing it. TYP_MASK locals should not be poisoned, because they should not be address exposed.

Code for both scenarios is emitted in the first basic block of the function.

TYP_SIMD/TYP_MASK locals are zero-initialized by writing the entire
UnknownSizeFrame through a short loop of vector stores.

TYP_SIMD locals are poisoned individually for debugging code, by
broadcasting the poison value to a vector and storing it. TYP_MASK
locals should not be poisoned, because they should not be address
exposed.

Code for both scenarios is emitted in the first basic block of the
function.
@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 13, 2026
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label May 13, 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.

@snickolls-arm

Copy link
Copy Markdown
Contributor Author

Hi @jakobbotsch, please could I have a review? I'm not sure what's going on with the superpmi-replay, possibly the MCH files haven't been generated yet for this version.

@a74nh a74nh mentioned this pull request Jun 24, 2026
20 tasks
@snickolls-arm

Copy link
Copy Markdown
Contributor Author

@dotnet/arm64-contrib Please could I have a review?

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

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "d05812a56f0f8354c42fff0b1642848cdebc267e",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "8489f3259c2f22d70328fb390611a5326e7dafc0",
  "last_reviewed_commit": "d05812a56f0f8354c42fff0b1642848cdebc267e",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "8489f3259c2f22d70328fb390611a5326e7dafc0",
  "last_recorded_worker_run_id": "29679141190",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "d05812a56f0f8354c42fff0b1642848cdebc267e",
      "review_id": 4730526487
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: Real and well-scoped. SVE TYP_SIMD/TYP_MASK locals live on the Arm64 UnknownSizeFrame, whose size is unknown at compile time, so the generic frame zero-init and poisoning paths (which assume a known byte size and fp/sp-relative addressing) cannot handle them. This PR adds the missing VL-relative zero-initialization and (best-effort) poisoning for that frame.

Approach: Reasonable and consistent with the surrounding SVE codegen. Zero-init emits an unrolled run of str z9, [sp, #i MUL VL] for small frames and a addvl/str/cmp/b.ne countdown loop for larger ones, moved into the first BB right after the prolog (alongside the existing poison path). REG_SCRATCH/REG_SCRATCH_V are correctly registered as killed in buildIntervals, and emitIns_R_S gains an out-of-range-immediate fallback (rdvl+madd) for INS_lea. The loop bounds look correct: both variants cover exactly [sp, x19).

Summary: ⚠️ Needs Human Review. The change is internally consistent and the codegen appears correct, but this is an in-progress SVE feature with one substantive open point already under active maintainer discussion (see below). A human reviewer with Arm64/SVE codegen expertise should confirm the poisoning contract and the unconditional zero-init cost.


Detailed Findings

⚠️ warning — noway_assert(varDsc->TypeGet() == TYP_SIMD) in genPoisonUnknownSizeVariable (src/coreclr/jit/codegenarm64.cpp). The function asserts the local is TYP_SIMD, but the JIT can mark a local address-exposed (and therefore a poison candidate) for reasons that are not tied to it being a vector, so an address-exposed TYP_MASK local reaching this path would trigger a noway_assert — a fatal error even in release. This is already flagged and discussed inline by @jakobbotsch (the author agreed to either add a small memset-style poison routine for masks or skip mask poisoning entirely), so I am not filing a duplicate inline comment; calling it out here only to make sure it is resolved before merge rather than left as an implicit invariant.

💡 suggestion — unconditional zero-init (genZeroInitializeUnknownSizeFrame, src/coreclr/jit/codegenlinear.cpp). Zero-init is emitted whenever compUsesUnknownSizeFrame, independent of whether any of the SVE locals/temps on that frame actually require zeroing. For frames dominated by non-GC-tracked temporaries this may zero more than strictly necessary. If the whole UnknownSizeFrame genuinely must be zeroed for GC/verifiability reasons this is fine — worth a one-line comment stating that invariant so future readers don't mistake it for an unconditional over-zeroing.

💡 suggestion — empty doc-comment banner. genPoisonUnknownSizeVariable is introduced with an empty // header banner while its sibling genZeroInitializeUnknownSizeFrame has a full description. Consider giving it a matching short header for consistency. Non-blocking.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 106.6 AIC · ⌖ 10.7 AIC · ⊞ 10K

Comment thread src/coreclr/jit/codegenarm64.cpp Outdated
Comment thread src/coreclr/jit/emitarm64.cpp Outdated
@jakobbotsch
jakobbotsch merged commit f54b55b into dotnet:main Jul 28, 2026
141 of 143 checks passed
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 11.0-rc1 milestone Jul 29, 2026
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 community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants