From 91ff6b696bc668dcc7f358eed196741566dc4541 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Thu, 30 Jul 2026 12:13:18 +0000 Subject: [PATCH] fix(examine): encode GC version minor/revision as hex digits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/rocm-core/src/lib.rs | 87 ++++++++++++++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 6 deletions(-) diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 1300fb21..0c3985a0 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -3747,15 +3747,21 @@ fn parse_linux_kfd_gfx_target(value: &str) -> Option { } match digits.len() { 3 | 4 => Some(format!("gfx{digits}")), + // A 5/6-digit value is KFD's *packed* version (major·10000 + minor·100 + + // step), not a target name, so there is no `gfx{digits}` fallback here. + // Fabricating one from a version that failed to decode fed + // `gfx90010`-shaped tokens into `normalize_therock_family`, where the + // loose `gfx90` arm mapped them to a plausible-looking but wrong family. + // Yielding `None` instead lets detection try the next KFD node and then + // `ip_discovery`, and otherwise report the target as unknown — which is + // recoverable with `--family`, where a wrong family silently installs + // the wrong runtime wheel. 5 | 6 => { let raw: u32 = digits.parse().ok()?; let major = raw / 10_000; let minor = (raw / 100) % 100; let revision = raw % 100; - if let Some(token) = gfx_target_from_gc_version(major, minor, revision) { - return Some(token); - } - Some(format!("gfx{digits}")) + gfx_target_from_gc_version(major, minor, revision) } _ => None, } @@ -3835,11 +3841,26 @@ fn detect_ip_discovery_gc_target(ip_discovery_dir: &Path) -> Option { } #[cfg(any(target_os = "linux", test))] +/// Build the LLVM gfx target for a GC (Graphics Core) IP version. +/// +/// The target is `gfx` + major in decimal + minor and revision as **single hex +/// digits** — `gfx90a` is GC 9.0.**10** (`10 == 0xa`), and `gfx1250` is GC +/// 12.5.0. Concatenating the components as decimal agrees with hex only while +/// every component is below 10, so it silently produced `gfx9010` for gfx90a +/// hardware (MI210/MI250), which then normalized to the wrong TheRock family. +/// +/// A minor or revision that cannot be a single hex digit does not describe any +/// gfx target, so it yields `None` rather than a fabricated token: the caller +/// tries the next detection source and otherwise reports the target as unknown, +/// which is recoverable with `--family`, where a fabricated one silently +/// installs the wrong runtime wheel. `major` is only checked for zero — it is +/// printed in decimal and has no single-digit bound (`gfx1030`, `gfx1250`), so +/// an implausible major still concatenates into a token. fn gfx_target_from_gc_version(major: u32, minor: u32, revision: u32) -> Option { - if major == 0 { + if major == 0 || minor > 0xf || revision > 0xf { return None; } - Some(format!("gfx{major}{minor}{revision}")) + Some(format!("gfx{major}{minor:x}{revision:x}")) } #[derive(Debug, Clone, Copy, Serialize, Deserialize, Eq, PartialEq)] @@ -6891,6 +6912,42 @@ Class Name: Display gfx_target_from_gc_version(11, 0, 3), Some("gfx1103".to_owned()) ); + // Two digits of major stay decimal. + assert_eq!( + gfx_target_from_gc_version(12, 5, 0), + Some("gfx1250".to_owned()) + ); + } + + #[test] + fn gc_version_encodes_components_above_nine_as_hex_digits() { + // GC 9.0.10 is gfx90a (MI210/MI250), not "gfx9010": minor and revision + // are single hex digits. Decimal concatenation agreed with hex only + // while every component stayed below 10. + assert_eq!( + gfx_target_from_gc_version(9, 0, 10), + Some("gfx90a".to_owned()) + ); + assert_eq!( + gfx_target_from_gc_version(9, 4, 12), + Some("gfx94c".to_owned()) + ); + + // A component that is not a single hex digit describes no gfx target, + // so detection falls through rather than acting on a fabricated one. + assert_eq!(gfx_target_from_gc_version(12, 16, 0), None); + assert_eq!(gfx_target_from_gc_version(12, 0, 16), None); + assert_eq!(gfx_target_from_gc_version(0, 0, 1), None); + } + + #[test] + fn gfx90a_gc_version_normalizes_to_its_own_therock_family() { + // The point of the fix, end to end: "gfx9010" missed every specific arm + // of `normalize_therock_family` and fell through to the loose `gfx90` + // one, yielding "gfx90X-dcgpu" — the wrong runtime wheel, and outside + // `VLLM_PREFERRED_THEROCK_FAMILIES`, so engine selection changed too. + let target = gfx_target_from_gc_version(9, 0, 10).expect("gfx90a target"); + assert_eq!(normalize_therock_family(&target), Some("gfx90a".to_owned())); } #[test] @@ -6907,6 +6964,15 @@ Class Name: Display parse_linux_kfd_gfx_target("gfx1103"), Some("gfx1103".to_owned()) ); + // KFD packs gfx90a as 9·10000 + 0·100 + 10. + assert_eq!( + parse_linux_kfd_gfx_target("90010"), + Some("gfx90a".to_owned()) + ); + // A packed version whose components are not single hex digits is not a + // target; it must not be passed through as `gfx{digits}`, which used to + // normalize to a plausible-looking but wrong family. + assert_eq!(parse_linux_kfd_gfx_target("121600"), None); assert_eq!(parse_linux_kfd_gfx_target("not-a-target"), None); } @@ -6928,6 +6994,15 @@ Class Name: Display detect_ip_discovery_gc_target(&root.join("ip_discovery")), Some("gfx1103".to_owned()) ); + + // The same defect reached this fallback path, not just the KFD one. + fs::write(gc.join("major"), "9")?; + fs::write(gc.join("minor"), "0")?; + fs::write(gc.join("revision"), "10")?; + assert_eq!( + detect_ip_discovery_gc_target(&root.join("ip_discovery")), + Some("gfx90a".to_owned()) + ); fs::remove_dir_all(root).ok(); Ok(()) }