https://github.com/accauble updated https://github.com/llvm/llvm-project/pull/217449
>From b6e2ea160e852ac65646a3e656db5c9bb3d35c9a Mon Sep 17 00:00:00 2001 From: Allyson Cauble-Chantrenne <[email protected]> Date: Wed, 19 Aug 2026 15:18:48 -0400 Subject: [PATCH 1/3] [offload-arch] Report failure when KFD topology cannot be read printGPUsByKFD() did not check the error code until looping over a `directory_iterator`. This works fine unless the `directory_iterator` fails to construct, which is possible if the KFD topology cannot be read (e.g., in WSL). This meant that printAMD() would get a success but no GPU, when it should instead have gotten a failure and fallen back on printGPUsByHIP(). This fixes that by checking the error code after constructing the `directory_iterator`. This also adds a number of tests surrounding the printGPUsByKFD() that check to see if there is still success if there are GPUs or if there are no GPUs at all. --- clang/tools/offload-arch/AMDGPUArchByKFD.cpp | 17 +++- clang/unittests/offload-arch/CMakeLists.txt | 25 +++-- .../offload-arch/OffloadArchTest.cpp | 96 +++++++++++++++++++ 3 files changed, 127 insertions(+), 11 deletions(-) diff --git a/clang/tools/offload-arch/AMDGPUArchByKFD.cpp b/clang/tools/offload-arch/AMDGPUArchByKFD.cpp index 94ebf9073e00e..1334529a7daeb 100644 --- a/clang/tools/offload-arch/AMDGPUArchByKFD.cpp +++ b/clang/tools/offload-arch/AMDGPUArchByKFD.cpp @@ -29,11 +29,20 @@ constexpr static long getMajor(long Ver) { return (Ver / 10000) % 100; } constexpr static long getMinor(long Ver) { return (Ver / 100) % 100; } constexpr static long getStep(long Ver) { return Ver % 100; } -int printGPUsByKFD() { +// Enumerate the GPUs described by the KFD topology rooted at \p NodePath. +// Exposed so unit tests can run against a synthetic topology; it is not +// declared in a header. +int printGPUsByKFD(StringRef NodePath) { SmallVector<std::pair<long, long>> Devices; std::error_code EC; - for (sys::fs::directory_iterator Begin(KFD_SYSFS_NODE_PATH, EC), End; - Begin != End; Begin.increment(EC)) { + sys::fs::directory_iterator Begin(NodePath, EC), End; + + // Check if we could construct the directory_iterator, which can fail if + // there is no KFD driver (e.g., WSL) + if (EC) + return 1; + + for (; Begin != End; Begin.increment(EC)) { if (EC) return 1; @@ -75,3 +84,5 @@ int printGPUsByKFD() { return 0; } + +int printGPUsByKFD() { return printGPUsByKFD(KFD_SYSFS_NODE_PATH); } diff --git a/clang/unittests/offload-arch/CMakeLists.txt b/clang/unittests/offload-arch/CMakeLists.txt index db4fa5ceba5ea..8d9cbf5c60205 100644 --- a/clang/unittests/offload-arch/CMakeLists.txt +++ b/clang/unittests/offload-arch/CMakeLists.txt @@ -1,10 +1,19 @@ +set(OffloadArchTestSources + OffloadArchTest.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/../../tools/offload-arch/AMDGPUArchByKFD.cpp + ) + if(CMAKE_SYSTEM_NAME STREQUAL "Windows") - add_distinct_clang_unittest(OffloadArchTests - OffloadArchTest.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/../../tools/offload-arch/AMDGPUArchByHIP.cpp - CLANG_LIBS - clangBasic - LLVM_COMPONENTS - Support - ) + list(APPEND OffloadArchTestSources + ${CMAKE_CURRENT_SOURCE_DIR}/../../tools/offload-arch/AMDGPUArchByHIP.cpp) endif() + +add_distinct_clang_unittest(OffloadArchTests + ${OffloadArchTestSources} + CLANG_LIBS + clangBasic + LINK_LIBS + LLVMTestingSupport + LLVM_COMPONENTS + Support + ) diff --git a/clang/unittests/offload-arch/OffloadArchTest.cpp b/clang/unittests/offload-arch/OffloadArchTest.cpp index 4b07af39a918a..a120428bbde2d 100644 --- a/clang/unittests/offload-arch/OffloadArchTest.cpp +++ b/clang/unittests/offload-arch/OffloadArchTest.cpp @@ -9,8 +9,13 @@ #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/CommandLine.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/Path.h" +#include "llvm/Support/raw_ostream.h" +#include "llvm/Testing/Support/SupportHelpers.h" #include "gtest/gtest.h" #include <algorithm> +#include <cstdio> #include <string> // Defined in AMDGPUArchByHIP.cpp (non-static, compiled into this test). @@ -19,6 +24,9 @@ bool compareVersions(llvm::StringRef A, llvm::StringRef B); llvm::SmallVector<std::string, 8> getCandidateBinPaths(llvm::StringRef ExeDir); #endif +// Defined in AMDGPUArchByKFD.cpp (non-static, compiled into this test). +int printGPUsByKFD(llvm::StringRef NodePath); + using namespace llvm; cl::opt<bool> Verbose("offload-arch-test-verbose", cl::Hidden, cl::init(false)); @@ -112,3 +120,91 @@ TEST(CandidateBinPaths, NoDriveRootBin) { } #endif // _WIN32 + +// --- printGPUsByKFD --- + +namespace { +// Write <Dir>/<Node>/properties containing the given lines. +void addNode(StringRef Dir, unsigned Node, StringRef Properties) { + SmallString<128> NodeDir(Dir); + sys::path::append(NodeDir, Twine(Node)); + ASSERT_FALSE(sys::fs::create_directories(NodeDir)); + + SmallString<128> PropertiesPath(NodeDir); + sys::path::append(PropertiesPath, "properties"); + std::error_code EC; + raw_fd_ostream OS(PropertiesPath, EC); + ASSERT_FALSE(EC); + OS << Properties; +} + +// Write a node describing a GPU with the given gfx_target_version. +void addGPUNode(StringRef Dir, unsigned Node, StringRef GFXVersion) { + addNode(Dir, Node, ("gfx_target_version " + GFXVersion + "\n").str()); +} + +// Run printGPUsByKFD, collecting what it writes to stdout. +int printGPUsByKFDCapturingStdout(StringRef NodePath, std::string &Output) { + testing::internal::CaptureStdout(); + int Result = printGPUsByKFD(NodePath); + std::fflush(stdout); + Output = testing::internal::GetCapturedStdout(); + return Result; +} +} // namespace + +// A topology directory that cannot be opened must be reported as a failure, so +// that the caller falls back to enumerating with the HIP runtime. +TEST(KFDTopology, MissingDirectoryFails) { + unittest::TempDir Dir("kfd-topology", /*Unique=*/true); + std::string Output; + EXPECT_EQ(printGPUsByKFDCapturingStdout(Dir.path("does-not-exist"), Output), + 1); + EXPECT_EQ(Output, ""); +} + +// A readable topology describing no GPUs is not an error, and prints nothing. +TEST(KFDTopology, CPUOnlyTopologySucceeds) { + unittest::TempDir Dir("kfd-topology", /*Unique=*/true); + addGPUNode(Dir.path(), 0, "0"); + std::string Output; + EXPECT_EQ(printGPUsByKFDCapturingStdout(Dir.path(), Output), 0); + EXPECT_EQ(Output, ""); +} + +// A node whose properties do not mention gfx_target_version is a CPU too. +TEST(KFDTopology, NodeWithoutGFXVersionSucceeds) { + unittest::TempDir Dir("kfd-topology", /*Unique=*/true); + addNode(Dir.path(), 0, "cpu_cores_count 16\n"); + std::string Output; + EXPECT_EQ(printGPUsByKFDCapturingStdout(Dir.path(), Output), 0); + EXPECT_EQ(Output, ""); +} + +TEST(KFDTopology, EmptyTopologySucceeds) { + unittest::TempDir Dir("kfd-topology", /*Unique=*/true); + std::string Output; + EXPECT_EQ(printGPUsByKFDCapturingStdout(Dir.path(), Output), 0); + EXPECT_EQ(Output, ""); +} + +TEST(KFDTopology, GPUNodeIsPrinted) { + unittest::TempDir Dir("kfd-topology", /*Unique=*/true); + addGPUNode(Dir.path(), 0, "0"); // CPU + addGPUNode(Dir.path(), 1, "110001"); // gfx1101 + std::string Output; + EXPECT_EQ(printGPUsByKFDCapturingStdout(Dir.path(), Output), 0); + EXPECT_EQ(Output, "gfx1101\n"); +} + +// Devices are printed in node order, and the step is printed in hex so that +// e.g. gfx90a renders correctly. +TEST(KFDTopology, MultipleGPUsArePrintedInNodeOrder) { + unittest::TempDir Dir("kfd-topology", /*Unique=*/true); + addGPUNode(Dir.path(), 0, "0"); // CPU + addGPUNode(Dir.path(), 2, "90010"); // gfx90a + addGPUNode(Dir.path(), 1, "110001"); // gfx1101 + std::string Output; + EXPECT_EQ(printGPUsByKFDCapturingStdout(Dir.path(), Output), 0); + EXPECT_EQ(Output, "gfx1101\ngfx90a\n"); +} >From 7d2ef583f886d310fe0846747049f4a08a73b552 Mon Sep 17 00:00:00 2001 From: Allyson Cauble-Chantrenne <[email protected]> Date: Thu, 20 Aug 2026 10:33:49 -0400 Subject: [PATCH 2/3] [offload-arch] Moved AMDGPUArchByKFD and tests to llvm streams --- clang/tools/offload-arch/AMDGPUArchByKFD.cpp | 11 +++++++---- clang/unittests/offload-arch/OffloadArchTest.cpp | 3 +-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/clang/tools/offload-arch/AMDGPUArchByKFD.cpp b/clang/tools/offload-arch/AMDGPUArchByKFD.cpp index 1334529a7daeb..2ac8dd7c75e7f 100644 --- a/clang/tools/offload-arch/AMDGPUArchByKFD.cpp +++ b/clang/tools/offload-arch/AMDGPUArchByKFD.cpp @@ -13,9 +13,11 @@ //===----------------------------------------------------------------------===// #include "llvm/Support/FileSystem.h" +#include "llvm/Support/Format.h" #include "llvm/Support/LineIterator.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/Path.h" +#include "llvm/Support/raw_ostream.h" #include <memory> using namespace llvm; @@ -37,8 +39,9 @@ int printGPUsByKFD(StringRef NodePath) { std::error_code EC; sys::fs::directory_iterator Begin(NodePath, EC), End; - // Check if we could construct the directory_iterator, which can fail if - // there is no KFD driver (e.g., WSL) + // Fail if the sysfs topology does not exist so that the caller can fall + // back to the HIP runtime. This can happen when the amdgpu kernel module + // is not loaded (e.g., WSL). if (EC) return 1; @@ -79,8 +82,8 @@ int printGPUsByKFD(StringRef NodePath) { // Sort the devices by their node to make sure it prints in order. llvm::sort(Devices, [](auto &L, auto &R) { return L.first < R.first; }); for (const auto &[Node, GFXVersion] : Devices) - std::fprintf(stdout, "gfx%ld%ld%lx\n", getMajor(GFXVersion), - getMinor(GFXVersion), getStep(GFXVersion)); + outs() << "gfx" << getMajor(GFXVersion) << getMinor(GFXVersion) + << format_hex_no_prefix(getStep(GFXVersion), 1) << '\n'; return 0; } diff --git a/clang/unittests/offload-arch/OffloadArchTest.cpp b/clang/unittests/offload-arch/OffloadArchTest.cpp index a120428bbde2d..5f5e49f5c72cc 100644 --- a/clang/unittests/offload-arch/OffloadArchTest.cpp +++ b/clang/unittests/offload-arch/OffloadArchTest.cpp @@ -15,7 +15,6 @@ #include "llvm/Testing/Support/SupportHelpers.h" #include "gtest/gtest.h" #include <algorithm> -#include <cstdio> #include <string> // Defined in AMDGPUArchByHIP.cpp (non-static, compiled into this test). @@ -147,7 +146,7 @@ void addGPUNode(StringRef Dir, unsigned Node, StringRef GFXVersion) { int printGPUsByKFDCapturingStdout(StringRef NodePath, std::string &Output) { testing::internal::CaptureStdout(); int Result = printGPUsByKFD(NodePath); - std::fflush(stdout); + outs().flush(); Output = testing::internal::GetCapturedStdout(); return Result; } >From 3541acd3977a2b9f5b27b4af016aa8e721a166d6 Mon Sep 17 00:00:00 2001 From: Allyson Cauble-Chantrenne <[email protected]> Date: Fri, 21 Aug 2026 06:29:33 -0400 Subject: [PATCH 3/3] [offload-arch] Trimmed added comments in printGPUsByFKD --- clang/tools/offload-arch/AMDGPUArchByKFD.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/clang/tools/offload-arch/AMDGPUArchByKFD.cpp b/clang/tools/offload-arch/AMDGPUArchByKFD.cpp index 2ac8dd7c75e7f..8e4284eb5f8de 100644 --- a/clang/tools/offload-arch/AMDGPUArchByKFD.cpp +++ b/clang/tools/offload-arch/AMDGPUArchByKFD.cpp @@ -31,17 +31,13 @@ constexpr static long getMajor(long Ver) { return (Ver / 10000) % 100; } constexpr static long getMinor(long Ver) { return (Ver / 100) % 100; } constexpr static long getStep(long Ver) { return Ver % 100; } -// Enumerate the GPUs described by the KFD topology rooted at \p NodePath. -// Exposed so unit tests can run against a synthetic topology; it is not -// declared in a header. +// Exposed for testing int printGPUsByKFD(StringRef NodePath) { SmallVector<std::pair<long, long>> Devices; std::error_code EC; sys::fs::directory_iterator Begin(NodePath, EC), End; - // Fail if the sysfs topology does not exist so that the caller can fall - // back to the HIP runtime. This can happen when the amdgpu kernel module - // is not loaded (e.g., WSL). + // Fail if the sysfs topology does not exist (e.g., WSL) if (EC) return 1; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
