Skip to content

[comgr][hotswap] Search every locally dead SGPR pair - #3604

Closed
harsh-amd wants to merge 11 commits into
ROCm:amd-stagingfrom
harsh-amd:users/harsh/hotswap-upstream-draft-03-all-sgpr
Closed

[comgr][hotswap] Search every locally dead SGPR pair#3604
harsh-amd wants to merge 11 commits into
ROCm:amd-stagingfrom
harsh-amd:users/harsh/hotswap-upstream-draft-03-all-sgpr

Conversation

@harsh-amd

@harsh-amd harsh-amd commented Jul 25, 2026

Copy link
Copy Markdown

Current review status

This draft must not be merged in its current form. Its historical head (ff599d5fc87454f7aa9bf146ef97aed225ae0a21) is stacked on the old control-flow-index branch and conflicts with current amd-staging. It also analyzes the immutable original decode, while #3605 review proved that an earlier patch may already have changed later NOP-sled bytes.

Required design before rebasing

The all-pair search should use one bounded per-function backward liveness solution over all numbered SGPRs, not rebuild a DenseMap<size_t, BitVector> for every patch site. The cache must be keyed by function range, MaxSgprs, and an explicit text-mutation generation. Every immediate .text writer must increment that generation; a generation mismatch invalidates the cached decode/CFG/use-def/liveness state. Opaque control flow, missing instruction boundaries, invalid ranges, and decode failures remain fail-closed. Memory should be contiguous and bounded to the current function rather than object-wide sparse maps.

Required validation

  • Current-text mutation regression from [comgr][hotswap] Make SGPR scratch and initial relay routing safe #3605.
  • Exhaustive pair selection, tuple/subregister uses and defs, loops, joins, opaque edges, malformed decode, range/overflow boundaries, wave32/wave64 VCC behavior, and byte-level idempotency.
  • Adversarial many-site test recording wall time and peak RSS, plus the full hotswap unit/LIT suites.
  • Corpus transition and mi400-3 smoke gates before undrafting.

Dependency/next step

Keep this PR draft. First make #3605 a focused current-staging prerequisite, then replace this conflicting branch with the generation-aware per-function solver as a clean dependent commit (or wait until #3605 lands so the upstream Files changed view is standalone).

#3598 remains unchanged as the integration reference.

@paulzzy paulzzy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact current head ff599d5fc874 builds cleanly and passes its HotSwap unit/LIT tests, but it is not end-to-end safe yet.

In a translator-isolated A0 run, the hipSOLVER quick/smoke suite encountered a COMGR rewrite failure for an 8,348,656-byte gfx1250 code object (rc=1). ROCr then continued without a rewritten output, and three complex HETRD cases failed. The untouched image baseline passes all 11,746 hipSOLVER cases.

Candidate loading was explicitly confirmed, so this is not the earlier invalid global-COMGR replacement setup. Please reduce the failing object, identify whether the refusal belongs to this change or one of the cumulative prerequisites, and add it as an executable regression before making the PR ready. If it is inherited, the PR should be rebased after the prerequisite fix and this suite rerun.

@paulzzy paulzzy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exhaustive aligned-pair search is useful, but this implementation should not be carried forward as-is.

  • Its continuation proof analyzes the original Ctx.Decoded snapshot even after earlier patches mutate Ctx.Text, so it can reuse a pair that the current instruction stream reads.
  • The inherited VCC path still treats vcc_lo/vcc_hi partial definitions as full aggregate-VCC kills.
  • Ownerless commits mutate valid kernel statistics before a later malformed descriptor can fail, so repeated candidate attempts may leave metadata charges for a pair that was never selected.
  • Rebuilding the whole function and testing all 106 SGPRs at every exhausted site is approximately O(sites × instructions × 106).

The corrected live #3618 foundation already decodes current text, caches by mutation generation, tracks all SGPRs/VCC together, and handles partial VCC correctly. Corrected #3612 provides atomic/sticky ownerless metadata analysis. Please re-extract only #3604's policy on those foundations: union replacement incoming bits with continuation live bits once, check aggregate VCC, scan aligned pairs from s[104:105] through s[0:1], and commit through #3612.

Do not retain the duplicate numbered-SGPR walkers/scalar oracle. Add public regressions selecting s[0:1], ownerless local reuse, and %llvm-readelf metadata readback. The synthetic SGPR30_LO16 decoder state is not representative of a real gfx1250 MC operand and should not be used as production-safety evidence.

@lamb-j lamb-j added comgr Related to Code Object Manager hotswap Related to the Comgr Hotswap feature labels Jul 27, 2026
@harsh-amd harsh-amd closed this Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comgr Related to Code Object Manager hotswap Related to the Comgr Hotswap feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants