Improve JIT false-positive review guidance

[tannergooding]

Summary

Improve .github/instructions/jit.instructions.md with concise, actionable guidance that reduces common AI review false positives in core JIT code.

What this adds

• Clarifies TYP_I_IMPL/TYP_U_IMPL alias behavior and varType predicate expectations.
• Documents intentional JIT patterns that are often mis-flagged (retype-to-TYP_VOID rewrites, late-expansion temps, caller-proved lowering preconditions, spill flag semantics, selective flag recomputation).
• Adds review-hygiene guardrails (require phase/invariant context, avoid re-raising by-design claims without new evidence, avoid treating TODO/Cleanup comments as defects by default).

Scope

Instructions-file-only change. No product code changes.

Note

This PR description was generated with AI/Copilot assistance.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
15 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

Updates the JIT folder-specific review guidance (.github/instructions/jit.instructions.md) to document common AI/automated-review false positives and provide phase/invariant-focused review hygiene for src/coreclr/jit/** changes.

Changes:

• Adds a new “Common false-positive review mistakes” section with targeted bullets covering varType* contracts, common rewrite patterns, and flag semantics.
• Adds review-hygiene guardrails emphasizing phase/invariant context and avoiding re-raising already-resolved by-design discussions.

Show a summary per file

File	Description
.github/instructions/jit.instructions.md	Adds a consolidated list of common JIT review false positives and review-hygiene guidance for AI-assisted reviews.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

Suppressed comments (2)

.github/instructions/jit.instructions.md:15

• The guidance around TYP_I_IMPL/TYP_U_IMPL risks discouraging a valid review point: on 64-bit, TYP_I_IMPL expands to TYP_LONG, so varTypeIsInt(TYP_I_IMPL) is supposed to be false (32-bit-only). It would be clearer to spell out the target-dependent macro expansion and explicitly point reviewers to varTypeIsI/varTypeIsIntOrI when the intent is “native int”.

- **`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.

.github/instructions/jit.instructions.md:25

• The example new (compiler/allocator) isn’t a pattern that exists literally in the JIT codebase; the common forms are new (compiler, CMK_*) and sometimes new (compiler->getAllocator(...)). Using one of those here would make the guidance easier to map to real code.

- **JIT allocation is often arena-based.** Placement `new (compiler/allocator)` without `delete` is usually expected.

• Files reviewed: 1/1 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 0 new

[EgorBo]

Are these truly common mistakes made by the AI? I haven't noticed any of the listed myself. I assumed that when you correct an AI's mistake, it would store that correction as a shared memory for all contributors. Is that not the case? I do see such actions in the log my Copilot CLI time to time.

My main concern is duplicating documentation or including facts that might become outdated or simply redundant, which just consumes extra tokens. Should we consider adjusting our approach based on actual evidence of recorded mistakes?

[tannergooding]

Are these truly common mistakes made by the AI? I haven't noticed any of the listed myself.

I've hit many of these, especially the TYP_I_IMPL and varTypeIs ones both locally and in the automatic code review. They are ones that regularly have to be dismissed and are a bit dependent on the model that gets assigned; so are more prevalent for auto (which includes automatic code review).

I have personal instructions for several of these, but that doesn't help the code review or cloud agent skills that aren't run locally.

I assumed that when you correct an AI's mistake, it would store that correction as a shared memory for all contributors.

Copilot memory is a preview feature and is primarily on a personal basis: https://docs.github.com/en/copilot/concepts/agents/copilot-memory

While some preferences can be stored and shared, these can themselves quickly get out of date and eat up even more tokens or confuse the model. The current runtime set has around 100 and they aren't really well sorted and many even look suboptimal at a glance: https://github.com/dotnet/runtime/settings/copilot/memory

Explicit instructions are a stronger and better signal.

My main concern is duplicating documentation or including facts that might become outdated or simply redundant, which just consumes extra tokens. Should we consider adjusting our approach based on actual evidence of recorded mistakes?

AI cannot and should not be pulling in complex documentation every time, that's a much larger waste of tokens and more likely to get it "off" because it kills the context window. We really want a short and succinct list, which may get out of date and need to be updated over time.

It's infra, just like other parts of the codebase, and so we'll need to recheck instructions, skills, agents, and more when new models come out and older ones go away to clean things up.

[EgorBo]

AI cannot and should not be pulling in complex documentation every time,

Pretty sure such a large and popular OSS exists in pre-train so it doesn't even have to. I am not against common pitfalls listed in a skill, just personally never hit any of them, e.g **Liberal vs conservative VN differences are expected.**

but that doesn't help the code review or cloud agent skills that aren't run locally.

Right, hence, I asked for an evidence of non-local copilot mistakes.

Anyway, pinging the rest of the jit team if they hit any of those @dotnet/jit-contrib

PS: I personally try to remove as much noise (unrelated skills, mcp, general instructions) as I can while this change adds +1k tokens to the general context for any jit change if I understand it correctly.

[tannergooding]

Pretty sure such a large and popular OSS exists in pre-train so it doesn't even have to.

Being trained on it doesn't mean that there is enough weight/signal left after the model is made production ready. Most of it gets lost in the overall noise, which is why it still trivially gets things wrong without explicit instructions.

just personally never hit any of them

Here's an example of the varTypeIsIntOrI where review thinks it excludes TYP_LONG (and where I had to correct it on 3-4 iterations): #130481 (comment)

There's another draft PR I have up where it hits the TYP_I_IMPL doesn't work case, etc.

I'm happy to remove any of these if we don't think its enough value, but these are ones that I've seen getting hit and which the models themselves are calling out as points of confusion/contention, causing excess tokens or general failure to get it right.

[AndyAyersMS]

I've seen confusion on TYP_I_IMPL and (on Wasm) confusion about having a 32 bit target support TYP_LONG. But none of those others that I can recall.

One other issue I've seen a few times now:

• AI often makes claims about phase ordering that are wrong and very easily shown to be wrong, eg saying object allocation runs after morph. I have no idea why.

[EgorBo]

AI often makes claims about phase ordering that are wrong and very easily shown to be wrong, eg saying object allocation runs after morph. I have no idea why.

Yep, I've seen confusion about phase ordering as well. We should probably point to the function that calls all phases in order.

Here's an example of the varTypeIsIntOrI

Ok, I suggest we continue adding items to the list based on real evidence (links to bad reviews, screenshots of bad local behavior, or issues confirmed by multiple people on the team). I'd like to avoid potentially vibe-driven changes (not saying yours are) to the various prompt files, as I think a big chunk of our skills and that general review skills fall into that category.