Skip to content

fix(examine): encode GC version minor/revision as hex digits - #152

Open
volen-silo wants to merge 1 commit into
mainfrom
fix/gfx-target-gc-version-digits
Open

fix(examine): encode GC version minor/revision as hex digits#152
volen-silo wants to merge 1 commit into
mainfrom
fix/gfx-target-gc-version-digits

Conversation

@volen-silo

Copy link
Copy Markdown
Collaborator
  • If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No EAI-7346 rows exist.)

Problem

An LLVM gfx target is gfx + major in decimal + minor and revision as single hex digitsgfx90a is GC 9.0.10 (10 == 0xa), gfx1250 is GC 12.5.0. gfx_target_from_gc_version concatenated all three components as decimal, which agrees with hex only while every component is below 10.

Checked against every gfx target this repo already knows — gfx900/906/908/90a/942/950, gfx1030–1036, gfx1100–1103, gfx1150–1153, gfx1200/1201/1250 — and none contradicts the hex convention.

The concrete victim is gfx90a (MI210/MI250/MI250X), on the primary detection path (detect_linux_gfx_target tries KFD first, ip_discovery second — both feed the same helper):

  1. KFD packs gfx90a as 90010 (major·10000 + minor·100 + step).
  2. parse_linux_kfd_gfx_target decodes 9 / 0 / 10 — correctly.
  3. The helper produced "gfx9010" instead of "gfx90a".
  4. normalize_therock_family("gfx9010") misses every specific arm and falls through to the loose starts_with("gfx90") one → "gfx90X-dcgpu" instead of "gfx90a".
  5. Wrong TheRock family → wrong runtime wheel; and gfx90a drops out of VLLM_PREFERRED_THEROCK_FAMILIES, so engine selection changes too.

Approach

  • Format minor and revision as hex; major stays decimal (gfx1030, gfx1250 need both digits).
  • A minor or revision that cannot be a single hex digit yields None rather than a fabricated target, so detection tries the next source and otherwise reports the target as unknown.
  • The 5/6-digit branch of parse_linux_kfd_gfx_target no longer falls back to gfx{digits}. That value is KFD's packed version, not a target name, and passing it through was how out-of-range decodes still reached the loose family arm. The 3/4-digit branch keeps gfx{digits}, which is correct there.

Sub-10 components are unchanged, so every existing expectation holds.

Behavior change

An unknown target is recoverable with --family, but this does turn a previously-succeeding (and wrong-wheel) rocm install sdk on an affected host into a hard error from resolve_family, which lists the recognized families. That is the intended direction — failing loudly beats silently installing the wrong runtime — but it is a visible change.

What this does not close

Not Fixes EAI-7346, deliberately. The reported MI455X symptom (gfx1250 read as gfx1210) does not appear to follow from this code: GC 12.5.0 packs to 120500 and decodes to gfx1250 both before and after. Under the old encoding gfx1210 was also reachable from GC 1.2.10 and 1.21.0 (packed 10210 / 12100), which this change maps to gfx12a and None — but neither is a plausible GC IP version.

I could not obtain the MI455X sysfs values, so that is reasoning about the code, not an observation of the hardware. Closing EAI-7346 needs gfx_target_version and ip_discovery/die/*/GC/*/{major,minor,revision} from the machine. If the cause is the GC IP version differing from the LLVM target, the fix is an override consulted before or inside detect_linux_sysfs_gfx_target — the existing PCI/marketing table cannot serve, since it is only reached when sysfs returns None and, on Linux, only under WSL.

Related gap, not addressed here: normalize_therock_family("gfx1250") returns None, so even a correctly detected gfx1250 maps to no family. Naming MI455X's TheRock family would be a guess, and guessing reintroduces the silent-wrong-family class this PR fixes.

Testing

cargo fmt --all -- --check and cargo clippy --workspace --all-targets -- -D warnings clean. cargo test --workspace --no-fail-fast passes except two pre-existing rocm-core proc_lifecycle::tests::tree_* failures, which fail on WSL2 for process-tree signalling reasons unrelated to this change (untouched file).

New coverage: the gfx90a regression at the helper, through parse_linux_kfd_gfx_target("90010"), through an ip_discovery fixture, and end to end via normalize_therock_family — plus out-of-range rejection. Every new assertion except the major == 0 case fails against the old implementation.

Not run locally: no GPU on this host, so rocm examine was not exercised against real gfx90a hardware; the KFD and ip_discovery paths are covered by fixtures only.

Refs EAI-7346.

An LLVM gfx target is `gfx` + major in decimal + minor and revision as single
hex digits — `gfx90a` is GC 9.0.10 (10 == 0xa). `gfx_target_from_gc_version`
concatenated the three components as decimal, which agrees with hex only while
every component is below 10. Checked against every gfx target this repo already
knows (gfx900/906/908/90a/942/950, gfx1030-1036, gfx1100-1103, gfx1150-1153,
gfx1200/1201/1250); none contradicts the hex convention.

The concrete victim is gfx90a (MI210/MI250/MI250X), on the primary detection
path: KFD packs it as `90010`, `parse_linux_kfd_gfx_target` decodes 9/0/10
correctly, and the helper then produced "gfx9010". That token misses every
specific arm of `normalize_therock_family` and falls through to the loose
`starts_with("gfx90")` one, yielding "gfx90X-dcgpu" instead of "gfx90a" — the
wrong TheRock runtime wheel, and outside VLLM_PREFERRED_THEROCK_FAMILIES, so
engine selection changed too. The ip_discovery fallback fed the same helper and
had the same defect.

A minor or revision that cannot be a single hex digit now yields None rather
than a fabricated target, so detection tries the next source and otherwise
reports the target as unknown. For the same reason the 5/6-digit branch of
`parse_linux_kfd_gfx_target` no longer falls back to `gfx{digits}`: that value
is KFD's packed version, not a target name, and passing it through was how
out-of-range decodes still reached the loose family arm. An unknown target is
recoverable with `--family`; note this turns a previously-succeeding (but
wrong-wheel) `rocm install sdk` on such a host into a hard error from
`resolve_family`.

Sub-10 components are unchanged, so every existing expectation holds.

Refs EAI-7346. This does not appear to close the reported MI455X symptom
(gfx1250 read as gfx1210). GC 12.5.0 packs to 120500 and decodes to gfx1250
both before and after this change. Under the old encoding `gfx1210` was also
reachable from GC 1.2.10 and 1.21.0 (packed 10210 / 12100), which this change
maps to gfx12a and None respectively — but neither is a plausible GC IP
version. I could not obtain the MI455X sysfs values, so this is reasoning about
the code, not an observation of the hardware: closing EAI-7346 needs that data.
If the cause is the GC IP version differing from the LLVM target, the fix is an
override consulted before or inside `detect_linux_sysfs_gfx_target` — the
existing PCI/marketing table cannot serve, since it is only reached when sysfs
returns None and, on Linux, only under WSL.

Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
@volen-silo
volen-silo requested a review from a team as a code owner July 30, 2026 12:59
@rominf

rominf commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Review

Approve — no blocking issues. The fix lands in the shared gfx_target_from_gc_version helper, so one change covers both the KFD and ip_discovery detection paths; I swept for a second decimal-concatenation decoder elsewhere and found none (the Windows/WSL path resolves via PCI-ID and marketing-name tables). Regression coverage at four levels for one bug, and the "what this does not close" section is the kind of disclosure that makes a fix trustworthy.

Verified locally: cargo fmt --all -- --check clean; cargo clippy --workspace --all-targets -- -D warnings clean; cargo test -p rocm-core --lib → 201 passed, 2 failed, both the pre-existing WSL2 proc_lifecycle process-tree failures in an untouched file. I also confirmed the claim that every new assertion except the major == 0 case fails against the old implementation — it holds exactly as stated.

1. The stated justification for removing the gfx{digits} fallback describes a path the old code could not take

This is the one I'd ask you to address before merge. It's comment-only, but it's the reason given for removing code, and it goes into public history.

The new block comment above the 5 | 6 => arm of parse_linux_kfd_gfx_target says the fallback "fed gfx90010-shaped tokens into normalize_therock_family, where the loose gfx90 arm mapped them to a plausible-looking but wrong family."

In the pre-change code the Some(format!("gfx{digits}")) fallback was reached only when gfx_target_from_gc_version returned None, and the old helper returned None only for major == 0. With digits.len() of 5 or 6 that requires a leading-zero string like "009010" — so for every realistic KFD gfx_target_version the fallback was effectively unreachable. The mechanism that actually produced the wrong family was the decimal concatenation in the helper: (9,0,10)"gfx9010" → loose starts_with("gfx90") arm → gfx90X-dcgpu. Never gfx90010.

The test comment on parse_linux_kfd_gfx_target("121600") is wrong in a second way: under the old code that returned Some("gfx12160") from the concat, not gfx{digits}, and normalize_therock_family("gfx12160") matched no arm and already returned None — so it never normalized to a wrong family either. The assertion itself is still worth keeping (it pins the new bound end-to-end); only the comment needs correcting.

Worth noting this also slightly overstates the change: the fallback removal is inert except for leading-zero inputs. The real behavior change — previously-succeeding hosts now getting None — comes from the new minor > 0xf || revision > 0xf bound in the helper. Same correction applies to the commit body and the PR description sentence "passing it through was how out-of-range decodes still reached the loose family arm."

2. gfx94c is synthetic but reads as real

In gc_version_encodes_components_above_nine_as_hex_digits, gfx94c is not a shipped part and appears nowhere else in the tree — fine for exercising a second hex digit beyond a on a pure formatting function, but the adjacent gfx90a case is annotated as real MI210/MI250 hardware, so the unannotated one reads as equally real. A one-liner (// not a shipped part — chosen only to exercise a hex digit other than 'a') avoids a reader mining tests for supported targets and picking up a nonexistent one.

3. Doc comment placement

The new /// block sits below #[cfg(any(target_os = "linux", test))]. Legal, but the file has 12 instances of /// immediately preceding #[cfg(...)] and, after this PR, exactly one of the reverse.

4. Two untested mechanisms this PR makes load-bearing (follow-up)

The safety argument for returning None rests on two behaviors nothing exercises:

  • Fall-through on None — the KFD node loop and DRM card loop continue past a node that now yields None. Before this PR a malformed node almost always yielded something, so the continue was rarely the deciding factor; now it is. No test builds two nodes (one out-of-range, one valid) and asserts the valid one wins.
  • The new hard-error pathresolve_family's bail! is untested. I read the message and it is genuinely actionable (names --family, lists known_therock_families(), appends detect_host_gpu_diagnostics()), but nothing pins that.

Both look correct on inspection; non-blocking given how strong the coverage of the actual defect is.

5. Diagnostics don't surface the raw GC triple on failure (follow-up)

When detection now returns None, detect_host_gpu_diagnostics re-derives the target and prints detected_gfx_target: <unknown> — the major/minor/revision (or the packed gfx_target_version) that caused the rejection appear nowhere. Pre-existing shape, but this PR is what makes hitting that path expected, and per your own scope note the missing MI455X sysfs values are exactly what's needed next. Echoing the raw values would let affected users self-report that data.

6. normalize_therock_family still has no arm for gfx90c or gfx1250 (pre-existing)

Now that the encoder emits real hex-bearing names, gfx90c (Renoir/Cezanne APUs, GC 9.0.12) resolves correctly and then falls through the loose starts_with("gfx90") arm to gfx90X-dcgpu — a datacenter family for an APU. Not a regression (old "gfx9012" hit the same arm), just newly visible. The loose catch-all is the same mechanism behind this bug and will keep producing plausible-but-wrong families for any gfx9 target without a specific arm. Correctly not bundled here — worth its own issue proposing explicit arms or a known-target table.

Tradeoffs, surfaced rather than contested

  • None → hard error. I traced the user-visible result: clean and actionable, not a panic. Right direction, and the PR flags it rather than burying it — but it is a user-visible regression in "it used to work" terms for affected hosts, so worth being a conscious maintainer call.
  • Bounding minor/revision but not major is coherent per the LLVM convention and the doc comment says so. A typed GcVersion with TryFrom, or validation against a known-targets table so a syntactically-valid-but-nonexistent triple like (12,5,1)gfx1251 isn't reported as "detected" only to dead-end at family resolution, would be a stronger contract. Correctly deferred.
  • Scoping out the MI455X symptom — confirmed independently that gfx_target_from_gc_version(12, 5, 0) yields gfx1250 both before and after, so this is a genuine no-op there. Anyone tracking that work shouldn't read this merging as closing it.

Release notes

Two user-visible changes worth calling out:

  • On a host whose GC version decodes to an out-of-range minor/revision, rocm install sdk now fails with unable to resolve a supported TheRock GPU family for this host instead of silently installing a wrong-family runtime wheel. Recovery is --family <FAMILY>.
  • gfx90a hosts (MI210 / MI250 / MI250X) on the KFD or ip_discovery paths now resolve to family gfx90a instead of gfx90X-dcgpu. This re-admits gfx90a to VLLM_PREFERRED_THEROCK_FAMILIES, so engine selection changes on those machines — relevant to anyone who has been working around the old behavior with an explicit --family or env override.

Nothing required outside the diff: no changelog in this repo, no expectations.toml update, no stale xfail rows, and no fixture under tests/e2e-cucumber/** encodes a fabricated token.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants