JIT: Tail-merge process all sets at once#129363
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
* move return/throw block dedulplication to before tail merge * add partition impl, but don't use it yet
* remove 'already processed' check as its no longer needed
106401d to
534ebff
Compare
|
@AndyAyersMS PTAL. The diffs are what I expected, with the exception of the tp regression on x86 - I have no clue why that is and I don't know a good way of finding out. The tiny assembly diffs are only because I also moved return/throw merging to happen before tail-merge and sometimes the |
…rge-process-sets-at-once
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "92a8a5f2e34073ffa02d1624adb649785686d5d2",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "6c5849144dc8a23aa0760080d6f7784fed60da37",
"last_reviewed_commit": "92a8a5f2e34073ffa02d1624adb649785686d5d2",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "6c5849144dc8a23aa0760080d6f7784fed60da37",
"last_recorded_worker_run_id": "29680716920",
"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": "92a8a5f2e34073ffa02d1624adb649785686d5d2",
"review_id": 4730527753
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: fgHeadTailMerge previously processed only one matching set of tail-merge candidates per call to tailMergePreds, then had the caller re-gather all candidates and retry (do { ... } while (tailMergePreds(nullptr)) and while (tailMerge(block))), skipping already-processed blocks with checks like !block->KindIs(BBJ_RETURN, BBJ_THROW) || block->isEmpty(). This repeated O(N) re-gathering was inefficient. The goal is to process all matching sets in a single pass.
Approach: The pred list moves from ArrayStack to jitstd::vector, and a new jitstd::partition helper (equivalent to std::partition) is added to algorithm.h. Instead of copying matches into a separate matchedPredInfo stack, tailMergePreds walks the vector once, using partition to make each matching set contiguous in [matchesBegin, matchesEnd) and advancing matchesBegin to the previous matchesEnd. tailMergePreds now returns an optimizedCount rather than a retry bool, and the outer driver loops (iterateTailMerge, the return/throw do/while) collapse into single passes plus the existing retryBlocks worklist.
Summary: This is a well-structured, self-contained JIT refactor that should be behavior-preserving for the set of merges performed while removing redundant re-scanning. The new partition implementation is correct (standard Hoare-style two-pointer partition over a half-open range, returning the first right-group element). The jitstd::vector migration and iterator-based rewrites are faithful to the original logic, and the madeChanges/optimizedCount bookkeeping is consolidated cleanly. One behavior question is raised inline regarding the cross-jump retry now being gated on hasCommSucc. Because this is an optimization-throughput change, the key validation is SPMI asmdiffs and JIT CI (including diff/no-diff and stress modes); reviewers should confirm those are green before merging. No correctness defects were found by reading the diff.
Detailed Findings
-
[Behavior] Cross-jump retry now gated on
hasCommSucc(src/coreclr/jit/fgopt.cpp, see inline comment near line 5411): The return/throw dedup pass (commSucc == nullptr) no longer pushescrossJumpTargetontoretryBlocks, whereas the previous iterativedo/whileeffectively re-examined return/throw blocks to a fixpoint. This is plausibly intentional and neutral, but should be confirmed via asmdiffs. -
[Style/nit]
jitstd::partitiondoc comment typos (src/coreclr/jit/jitstd/algorithm.h): "continous" and "swaping" are misspelled ("contiguous"/"swapping"); the same typos appear in thefgopt.cppcomment. Non-blocking. -
[Observation]
predInforeused across phases:predInfois now a single function-scoped vector cleared withclear()before each use (per-block, and for the return/throw batch). This is fine, but note the return/throw population path callsblock->lastStmt()directly (relying on the earlierblock->isEmpty()guard) rather than the NOP-skipping walk used intailMerge; this matches the prior code's behavior for that batch, so no change in semantics.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 118 AIC · ⌖ 10.8 AIC · ⊞ 10K
| // far we got through the pred list perhaps. | ||
| // | ||
| return true; | ||
| if (hasCommSucc) |
There was a problem hiding this comment.
Behavior change worth confirming: in the cross-jump path the retry is now scheduled only when hasCommSucc, so the return/throw dedup pass (commSucc == nullptr) no longer re-visits crossJumpTarget. The old code always pushed crossJumpTarget onto retryBlocks and, together with the previous do { ... } while (tailMergePreds(nullptr)) loop, iterated the return/throw merge until fixpoint. With the single-pass partition approach a split crossJumpTarget that gains a new matching last statement is no longer reconsidered against the remaining return/throw blocks. If SPMI asmdiffs show this is intentional/neutral, please note it; otherwise this may be a small optimization regression relative to the prior iterative behavior.
Say we call
tailMergePreds()with candidates:[A1, B2, C1, A3, B3, B1, A2].Previously, it only processed one set at a time - let's say all the A's.
Then we'd regather all candidates in the caller, skipping the ones which where already processed with checks like
!block->KindIs(BBJ_RETURN, BBJ_THROW) || block->isEmpty()and call it again until it finds no more.With this PR all sets are processed in a single call to
tailMergePreds.It works as follows. We start at index 0 (
A1) find the matches (all the other A's) and make them continous in memory, so we get:[A1, A3, A2, B2, C1, B3, B1]after the first call topartition. We then advance by the number of matches (3) and continue at B2. After the next call topartitionwe get:[A1, A3, A2, B2, B3, B1, C1].I didn't use the STL
std::partiton, as discussed.