https://github.com/accauble created https://github.com/llvm/llvm-project/pull/217449
## Motivation printGPUsByKFD() did not check the error code until looping over a `directory_iterator`. This worked 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(). ## Solution Checks the error code after constructing the `directory_iterator`. The actual fix is only a few lines in `clang/tools/offload-arch/AMDGPUArchByKFD.cpp`. ## Testing Adds some tests for printGPUsByKFD(). I may have gone overboard with the tests and I'm very okay with paring them back: * The important one for this fix is MissingDirectoryFails (makes sure the function fails if the directory doesn't exist) * There are two tests to make sure the function does not fail in the case there is only a CPU (CPUOnlyTopologySucceeds, NodeWithoutGFXVersionSucceeds) * There is a test that makes sure the function does not fail if the directory structure exists but is empty (EmptyTopologySucceeds) * Two tests to make sure that the function does not fail and actually finds GPUs (GPUNodeIsPrinted, MultipleGPUsArePrintedInNodeOrder) >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] [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"); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
