JIT: Cleanup and harden lowering

[tannergooding]

A batch of low-risk cleanup and hardening in JIT lowering, found during a lowering deep-dive. None of these are runtime-observable, so there are no regression tests. Grouped into focused commits.

---

lower.cpp -- debug-only diagnostics
SplitArgumentBetweenRegistersAndStack had a JITDUMP that evaluated splitPoint->GetOffset() on the branch where splitPoint == nullptr -- a verbose-only null-deref, and the format string didn't even consume it. Dropped that and the other unconsumed numRegs, stackSeg.Size args on the FIELD_LIST/BLK/reuse/spill dumps. Also fixed two [%06u} -> [%06u] typos in the containment diagnostics.

---

lowerarmarch.cpp -- shift/NOT containment hardening + dead stores
The child shift/NOT containment path in IsContainableUnaryOrBinaryOp didn't call IsInvariantInRange before allowing containment, unlike its siblings just above it. This isn't a live bug today (the operand is the immediate LIR predecessor by construction), but it's fragile and inconsistent, so this adds the matching guard and drops a redundant OperIs sub-condition already guaranteed by the enclosing block. Also removes dead stores to oper in LowerHWIntrinsic.

---

lowerxarch.cpp -- dead code + comment/typo fixes

• GetOperForHWIntrinsicId(&isScalar) clobbered the outer isScalar; the adjacent local userIsScalar was the intended target. Latent footgun, not a behavior change today.
• Removed a no-op node->Op(1) = op1;, a tautological assert(!src->TypeIs(TYP_STRUCT));, and dead + (0 * elemSize) arithmetic.
• Comment fixes: DPPD immediate 0x31 -> 0x33 to match the code, reworded the integer compare-less-than swap comment, Sse41.BlendVariable -> X86Base.BlendVariable, and assorted typos.

---

Verified: baseline build.cmd clr -rc checked succeeds, jitformat.py is clean, and the x64 JIT rebuilds with no warnings/errors. The lowerarmarch.cpp change is a verbatim structural mirror of adjacent compiling code in the same function.

Note

This PR description was drafted by GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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 does a small set of cleanup and hardening changes in CoreCLR JIT lowering, primarily removing dead/debug-only hazards and correcting internal comments/diagnostics.

Changes:

• Fix debug-only JITDUMP issues in lowering diagnostics (format typos, unused varargs, and a null-deref in a verbose-only path).
• Harden ARMArch shift containment under NOT/*_NOT by adding the same IsInvariantInRange guard used by adjacent containment paths; remove dead oper stores in ARM HWIntrinsic lowering.
• Remove no-op/dead code in xarch HWIntrinsic lowering, fix a latent isScalar out-param clobber, and update various comments/typos to match current behavior.

Show a summary per file

File	Description
src/coreclr/jit/lower.cpp	Corrects debug-only containment/split-arg diagnostics (format/args) and removes a verbose-only null-deref hazard.
src/coreclr/jit/lowerarmarch.cpp	Adds an IsInvariantInRange containment guard for shift+NOT cases and removes dead stores in HWIntrinsic lowering.
src/coreclr/jit/lowerxarch.cpp	Removes no-op/dead assertions/assignments, fixes GetOperForHWIntrinsicId out-param usage, and updates comments/typos for clarity and correctness.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "87528676b346b660386e93b9c1bb55cdf2f617f9",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "f2281d59e62aa6a75e53055b8f842bac108b9887",
  "last_reviewed_commit": "87528676b346b660386e93b9c1bb55cdf2f617f9",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "f2281d59e62aa6a75e53055b8f842bac108b9887",
  "last_recorded_worker_run_id": "29684894810",
  "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": "87528676b346b660386e93b9c1bb55cdf2f617f9",
      "review_id": 4730669898
    }
  ]
}

[github-actions]

Holistic Review

Motivation: A batch of low-risk cleanup and hardening in JIT lowering, uncovered during a lowering deep-dive. The goal is to remove dead code, fix latent footguns, correct misleading diagnostics, and align inconsistent containment logic—none of which is runtime-observable, so no regression tests accompany the change.

Approach: The PR groups focused edits across three lowering files. In lower.cpp it removes unconsumed JITDUMP arguments (including a verbose-only null-deref of splitPoint->GetOffset() on the splitPoint == nullptr branch) and fixes two [%06u} → [%06u] format typos. In lowerarmarch.cpp it adds a missing IsInvariantInRange guard to the shift/NOT containment path in IsContainableUnaryOrBinaryOp (mirroring the sibling GT_ADD/GT_SUB/... and GT_CMP/GT_OR/... blocks), drops an OperIs(GT_LSH, GT_RSH, GT_RSZ) sub-condition already guaranteed by the enclosing if, and removes dead stores to oper in LowerHWIntrinsic. In lowerxarch.cpp it fixes GetOperForHWIntrinsicId(&isScalar) → &userIsScalar (the outer isScalar was being clobbered), removes a no-op node->Op(1) = op1;, a tautological assert(!src->TypeIs(TYP_STRUCT)), dead + (0 * elemSize) arithmetic, and assorted comment/typo fixes.

Summary: I reviewed the full base-to-head range and confirmed each change. The removed oper assignments in the ARM LowerHWIntrinsic occur after the last read of oper (lines reading it at the GT_AND/GT_OR checks still see the value set at the top of the function), so the removal is safe. The new IsInvariantInRange guard is a verbatim structural mirror of the adjacent containment blocks in the same function and is strictly more conservative, so it cannot introduce a miscompile; the retained assert(shiftAmountNode->isContained()) matches the siblings. The &userIsScalar fix targets a genuinely misdirected out-parameter and does not change behavior today because the outer isScalar is not subsequently consumed in a way that depended on the clobbered value. The removed node->Op(1) = op1; is a no-op since op1 was just read from node->Op(1) with no intervening reassignment. All remaining edits are diagnostics, comments, and dead arithmetic with no functional effect. The changes are self-consistent, improve robustness, and carry negligible risk.

Verdict: LGTM. No actionable findings.

Note

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

Generated by Holistic Review · 66.6 AIC · ⌖ 10.6 AIC · ⊞ 10K

[tannergooding]

CC. @dotnet/jit-contrib, @EgorBo. Trivial cleanup of some comments, JITDUMP, and a simplification to a code path