From 89d3baf4ec807a8d2c5ef1ce9dbbebb2123a0d5d Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Fri, 31 Jul 2026 13:28:11 -0700 Subject: [PATCH 1/2] Improve JIT false-positive review guidance Expand .github/instructions/jit.instructions.md with concise, actionable guardrails to reduce common AI review false positives in core JIT areas. Highlights include: - clarifying TYP_I_IMPL/TYP_U_IMPL alias behavior and varType predicate expectations, - documenting intentional phase- and contract-driven patterns (retype-to-void rewrites, late expansion temps, caller-proved lowering preconditions, spill flag semantics), - adding review hygiene guidance to avoid re-raising by-design points without new evidence. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/instructions/jit.instructions.md | 26 ++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/.github/instructions/jit.instructions.md b/.github/instructions/jit.instructions.md index 3ee6dd7426d244..269c17997e0971 100644 --- a/.github/instructions/jit.instructions.md +++ b/.github/instructions/jit.instructions.md @@ -7,3 +7,29 @@ applyTo: "src/coreclr/jit/**" ## Code Review Guidelines - **Do not routinely request new targeted tests for pure refactors, mechanical cleanups, or other non-behavioral JIT codebase improvements.** JIT changes alter global codegen and are often validated by existing end-to-end suites and differential testing, so additional per-PR regression tests may not be needed for changes that do not affect observable behavior. However, targeted tests are appropriate and should be encouraged for bug fixes, changes with a clear repro, observable behavior changes, and previously untested edge cases that are not already covered by existing tests. + +## Common false-positive review mistakes + +- **`TYP_I_IMPL`/`TYP_U_IMPL` are aliases, not distinct types.** Do not claim `varTypeIsLong`/`varTypeIsInt` "miss" them. +- **`varTypeIsLong` is width-based.** It is not "C# `long` only." +- **Read `varType*` helpers by contract, not name.** Check `vartype.h` and target guards first. +- **Do not mix width and signedness.** `TYP_BYTE`/`TYP_UBYTE` and `TYP_SHORT`/`TYP_USHORT` differ by signedness; width helpers include both. +- **Debug-only checks are often intentional.** Flag only if release logic is actually missing. +- **Conservative patterns are often deliberate.** Require concrete evidence (miscompile, ordering break, or CQ regression). +- **Importer late-expansion temps can be intentional.** Do not auto-flag as lost side effects or unnecessary temps. +- **Retyping to `TYP_VOID` can be correct after rewrites.** Check the full transform and side-effect shape before flagging. +- **`GTF_VAR_MOREUSES` is a conservative hint.** Over-marking (for example during address materialization) is not a default correctness bug. +- **Lowering helpers can rely on caller-proved preconditions.** +- **`GTF_SPILL` and `GTF_SPILLED` are different.** Review with LSRA def/use and `GT_RELOAD` context; transient set/clear can be intentional. +- **Raw-copy node bashing can be intentional.** Verify replacement invariants before filing generic `memcpy` issues. +- **JIT allocation is often arena-based.** Placement `new (compiler/allocator)` without `delete` is usually expected. +- **Do not assume `|=` is always correct.** Some morph paths intentionally recompute flags with assignment to drop stale bits. +- **`GTF_DONT_CSE` is often intentional conservatism.** Do not remove/flag without proof. +- **`varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : genActualType(varDsc)` is established.** +- **Liberal vs conservative VN differences are expected.** +- **VN aliasing rules are deliberate model choices.** Validate against the documented VN memory model before flagging. +- **`unreached()`/asserting defaults on internal enums can be intentional.** Do not request graceful handling without evidence of a reachable path. +- **Prefer a question over a claim when phase context is unclear.** Ask where the invariant is established instead of filing a bug by default. +- **If a concern depends on a phase/invariant, name both.** No phase/invariant => no actionable review comment. +- **Do not treat TODO/Cleanup comments as defects by default.** Flag only with correctness risk or measured CQ impact. +- **Do not re-raise a claim already resolved as by-design.** If prior discussion explains intent, either accept it or provide new evidence. From c2057de0ec0b6bc81bb64556c366ad15fefb5a4b Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Fri, 31 Jul 2026 13:42:10 -0700 Subject: [PATCH 2/2] Clarify two JIT false-positive guardrails Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/instructions/jit.instructions.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/instructions/jit.instructions.md b/.github/instructions/jit.instructions.md index 269c17997e0971..429df13afd6dfa 100644 --- a/.github/instructions/jit.instructions.md +++ b/.github/instructions/jit.instructions.md @@ -10,7 +10,7 @@ applyTo: "src/coreclr/jit/**" ## Common false-positive review mistakes -- **`TYP_I_IMPL`/`TYP_U_IMPL` are aliases, not distinct types.** Do not claim `varTypeIsLong`/`varTypeIsInt` "miss" them. +- **`TYP_I_IMPL`/`TYP_U_IMPL` are target-dependent aliases, not distinct types.** They map to `TYP_LONG`/`TYP_ULONG` on 64-bit and `TYP_INT`/`TYP_UINT` on 32-bit. Do not claim `varTypeIsLong`/`varTypeIsInt` "miss" them; use `varTypeIsI`/`varTypeIsIntOrI` for native-int reasoning. - **`varTypeIsLong` is width-based.** It is not "C# `long` only." - **Read `varType*` helpers by contract, not name.** Check `vartype.h` and target guards first. - **Do not mix width and signedness.** `TYP_BYTE`/`TYP_UBYTE` and `TYP_SHORT`/`TYP_USHORT` differ by signedness; width helpers include both. @@ -22,7 +22,7 @@ applyTo: "src/coreclr/jit/**" - **Lowering helpers can rely on caller-proved preconditions.** - **`GTF_SPILL` and `GTF_SPILLED` are different.** Review with LSRA def/use and `GT_RELOAD` context; transient set/clear can be intentional. - **Raw-copy node bashing can be intentional.** Verify replacement invariants before filing generic `memcpy` issues. -- **JIT allocation is often arena-based.** Placement `new (compiler/allocator)` without `delete` is usually expected. +- **JIT allocation is often arena-based.** Placement `new (compiler, CMK_*)` or `new (compiler->getAllocator(...))` without `delete` is usually expected. - **Do not assume `|=` is always correct.** Some morph paths intentionally recompute flags with assignment to drop stale bits. - **`GTF_DONT_CSE` is often intentional conservatism.** Do not remove/flag without proof. - **`varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : genActualType(varDsc)` is established.**