Arm64: Initialization of TYP_SIMD/TYP_MASK locals - #128148
#128148Arm64: Initialization of TYP_SIMD/TYP_MASK locals#128148jakobbotsch merged 10 commits intodotnet:maindotnet/runtime:mainfrom snickolls-arm:local-initializationsnickolls-arm/runtime:local-initializationCopy head branch name to clipboard
Conversation
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.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
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. |
|
@dotnet/arm64-contrib Please could I have a review? |
|
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
}
]
} |
There was a problem hiding this comment.
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:
Detailed Findings
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
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.