[comgr][hotswap] Search every locally dead SGPR pair - #3604
Conversation
paulzzy
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
The exhaustive aligned-pair search is useful, but this implementation should not be carried forward as-is.
- Its continuation proof analyzes the original
Ctx.Decodedsnapshot even after earlier patches mutateCtx.Text, so it can reuse a pair that the current instruction stream reads. - The inherited VCC path still treats
vcc_lo/vcc_hipartial 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.
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 currentamd-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.textwriter 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
mi400-3smoke 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.