JIT: skip live-across-call CSEs with big footprint and few uses - #130931
JIT: skip live-across-call CSEs with big footprint and few uses#130931AndyAyersMS wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
|
Azure Pipelines: Successfully started running 6 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. |
The default CSE heuristic is overly aggressive for live-across-call CSE candidates with high size cost and very cold uses. This change is the outcome of back-propagating information from various RL-derived and oracular models for CSEs onto our existing heuristic, along with benchmark-derived validations for x64/arm64. The majority of CSEs blocked (on xArch) are large immediates. Note we generally don't CSE these on xArch (see eg dotnet#129941) but some handle kinds are allowed. These are fairly "cheap" to rematerialize. Other cases are some indirs and CSE-able calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e7aa92b to
d0ecf0d
Compare
|
@EgorBot -intel -amd -arm -linux_arm64 --filter "Perf_Regex_Cache.IsMatch_Multithreading" |
|
@EgorBot -intel -amd -arm -linux_arm64 --filter "Perf_Regex_Cache.IsMatch_Multithreading" |
|
@EgorBot -intel -amd -arm -linux_arm64 --filter 'Perf_Regex_Cache.IsMatch_Multithreading' |
|
@EgorBot -intel -amd -arm -linux_arm64 --filter 'Perf_Deep' |
|
@EgorBot -intel -amd -arm -linux_arm64 --filter 'TryGetValueTrueDictionary' --filter 'TryGetValueFalseDictionary' |
tip: you can ask ai to setup dotnet/performance locally to come up with filters that definitely work |
| // Reject CSE candidates that live across a call, have a non-trivial | ||
| // code footprint, and only a small (weighted) number of uses. | ||
| // | ||
| if ((CodeOptKind() != Compiler::SMALL_CODE) && candidate->LiveAcrossCall() && (candidate->Size() >= 8) && |
There was a problem hiding this comment.
should it depend on ABI/platform. I assume platforms with plenty of callee-saved regs would like to keep CSEing things like that?
There was a problem hiding this comment.
I had the modelling work on both x64 and arm64, and there didn't seem to be a big benefit to separate heuristics.
|
@EgorBot -intel -amd -arm -linux_arm64 --filter 'TryGetValueTrue.Dictionary' 'TryGetValueFalse.Dictionary' |
|
@EgorBot -intel -amd -arm -linux_arm64 --filter 'Collections.SortLinq*' |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "d0ecf0d431dbdd47ea3d9b8c4cd82693be3110ac",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "0bc487c793526e0ff826a57596e1d37e46c633d9",
"last_reviewed_commit": "d0ecf0d431dbdd47ea3d9b8c4cd82693be3110ac",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "0bc487c793526e0ff826a57596e1d37e46c633d9",
"last_recorded_worker_run_id": "29687175153",
"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": "d0ecf0d431dbdd47ea3d9b8c4cd82693be3110ac",
"review_id": 4730770515
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: The default CSE heuristic is overly aggressive for CSE candidates that live across a call, have a large code footprint, and only a few (weighted) uses. Such candidates add register pressure across calls (forcing spills/reloads) yet save little, and on xArch many are large immediates that are cheap to rematerialize. The change back-propagates insight from RL-derived/oracular CSE models onto the existing hand-written heuristic, validated by x64/arm64 benchmarks.
Approach: Adds an early rejection in CSE_Heuristic::PromotionCheck that returns false when CodeOptKind() != SMALL_CODE, the candidate is live across a call, its Size() (GetCostSz footprint) is >= 8, and its weighted UseCount() is <= 3. The guard is placed before the cost-model computation, so it purely narrows what gets promoted; it does not alter the subsequent cost math.
Summary: The change is small, self-contained, and correctly targeted. It only affects the default CSE_Heuristic (line 4359), not the Random/Replay/Parameterized (RL) variants, which have their own ConsiderCandidates/PromotionCheck paths — so experimental/policy-driven CSE flows are unaffected. Gating on != SMALL_CODE is appropriate because in SMALL_CODE mode UseCount()/Size() carry different (unweighted) semantics and the goal is size, not the register-pressure tradeoff this guard targets. The >= 8 and <= 3 thresholds are empirically derived magic constants; this matches the surrounding heuristic's established style, though it does mean future tuning requires re-derivation. No correctness risk: skipping a CSE is always semantically safe. The main risk is throughput/codegen quality regressions in edge cases, which the author states were validated against benchmarks and are covered by JIT CI (SPMI/asmdiffs). No test changes are needed for a pure heuristic tuning of this kind. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 79 AIC · ⌖ 10.4 AIC · ⊞ 10K
| if ((CodeOptKind() != Compiler::SMALL_CODE) && candidate->LiveAcrossCall() && (candidate->Size() >= 8) && | ||
| (candidate->UseCount() <= 3)) |
There was a problem hiding this comment.
LGTM.
Nit: the thresholds Size() >= 8 and UseCount() <= 3 came from the RL model and x64/arm64 sweep, a one-line comment on their derivation would help future re-tuning.
The default CSE heuristic is overly aggressive for live-across-call CSE candidates with high size cost and very cold uses.
This change is the outcome of back-propagating information from various RL-derived and oracular models for CSEs onto our existing heuristic, along with benchmark-derived validations for x64/arm64.
The majority of CSEs blocked (on xArch) are large immediates. Note we generally don't CSE these on xArch (see eg #129941) but some handle kinds are allowed. These are fairly "cheap" to rematerialize. Other cases are some indirs and CSE-able calls.