Add initial unit tests for jit gpu kernels #4278

pfultz2 · 2025-09-03T19:14:52Z

Motivation

Add initial unit tests for jit gpu kernels as we dont have any unit tests. This should cover the tests for #4194.

Technical Details

This adds a test driver to run unit tests. Currently it only works with one lane or all lanes being uniform, but we could extend it in the future to check across all lanes within a wavefront and show which lanes failed. I didnt implement this yet because its not needed right now.

Changelog Category

Improve test coverage(there isnt a category for this below).

- Added: New functionality.
- Changed: Changes to existing functionality.
- Removed: Functionality or support that has been removed. (Compared to a previous release)
- Optimized: Component performance that has been optimized or improved.
- Resolved Issues: Known issues from a previous version that have been resolved.
- Not Applicable: This PR is not to be included in the changelog.

Copilot

Pull Request Overview

This PR adds initial unit tests for JIT GPU kernels, providing a new test framework specifically designed for testing GPU kernel code. The main purpose is to improve test coverage for GPU kernel functionality, which previously had no unit testing infrastructure.

Creates a new GPU kernel testing framework with test driver and helper utilities
Adds comprehensive unit tests for array and algorithm functionality in GPU kernels
Implements a GPU-specific test harness that can compile and execute kernel tests on GPU

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
test/include/test.hpp	Adds callback mechanism for test case selection and refactors test execution
test/gpu/kernels/main.cpp	Main test driver for GPU kernel tests with compilation and execution logic
test/gpu/kernels/basic.cpp	Basic sanity tests for the GPU testing framework
test/gpu/kernels/array.cpp	Comprehensive tests for GPU array functionality
test/gpu/kernels/algorithm.cpp	Extensive tests for GPU algorithm implementations
test/gpu/kernels/CMakeLists.txt	Build configuration for GPU kernel tests
test/CMakeLists.txt	Integration of GPU kernel tests into main test suite
src/targets/gpu/kernels/include/migraphx/kernels/test.hpp	GPU-specific testing framework and macros
Multiple other files	Supporting changes for compilation, kernel management, and utilities

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-03T19:17:41Z

src/targets/gpu/kernels/include/migraphx/kernels/array.hpp

    template <class... Ts,
              MIGRAPHX_REQUIRES(sizeof...(Ts) == N and (is_convertible<Ts, T>{} and ...))>
-    constexpr array(Ts... xs) : d{xs...}
+    constexpr array(Ts... xs) : d{static_cast<value_type>(xs)...}


The explicit cast to value_type in the variadic constructor could cause silent truncation or precision loss. Consider using a concept or SFINAE to ensure safe conversions, or document the potential for data loss in conversions.

Copilot · 2025-09-03T19:17:41Z

test/gpu/kernels/main.cpp

+    {
+        auto failures = migraphx::gpu::write_to_gpu(int32_t{0}, true);
+        compile();
+        k.launch(nullptr, options.global, options.local)(test_cases.at(case_name), failures.get());


Using test_cases.at(case_name) could throw an exception if the case name is not found. Since this is in a test context, consider using find() with proper error handling or add a check to ensure the case exists before calling at().

Suggested change

k.launch(nullptr, options.global, options.local)(test_cases.at(case_name), failures.get());

auto it = test_cases.find(case_name);

if(it == test_cases.end())

{

std::cerr << "Test case not found: " << case_name << std::endl;

CHECK(false); // Fail the test

return;

}

k.launch(nullptr, options.global, options.local)(it->second, failures.get());

Copilot · 2025-09-03T19:17:42Z

test/gpu/kernels/main.cpp

+            if(suites.count(name) == 0)
+                continue;
+            run_suites.insert(suites.at(name));


The code performs two lookups in the suites map: first with count() and then with at(). Consider using find() once and checking the iterator, or use suites.find(name) to avoid the double lookup.

Suggested change

if(suites.count(name) == 0)

continue;

run_suites.insert(suites.at(name));

auto it = suites.find(name);

if(it == suites.end())

continue;

run_suites.insert(it->second);

Copilot · 2025-09-03T19:17:42Z

test/gpu/kernels/main.cpp

+    auto it = content.cbegin();
+    while(std::regex_search(it, content.cend(), m, case_re))
+    {
+        test_names.push_back(m[1].str());


Creating a temporary string with m[1].str() and then copying it into the vector could be inefficient. Consider using emplace_back(m[1].str()) or directly constructing the string in place if possible.

Suggested change

test_names.push_back(m[1].str());

test_names.emplace_back(m[1].str());

Copilot · 2025-09-03T19:17:42Z

src/targets/gpu/kernels/include/migraphx/kernels/test.hpp

+    friend constexpr Stream& operator<<(Stream& s, const lhs_expression& self)
+    {
+        const char* op = Operator::as_string();
+        if(op != nullptr or *op != '\0')


The condition op != nullptr or *op != '\0' has a logic error. If op is nullptr, dereferencing it with *op will cause undefined behavior. The condition should use and instead of or: if(op != nullptr and *op != '\0').

Suggested change

if(op != nullptr or *op != '\0')

if(op != nullptr && *op != '\0')

Copilot · 2025-09-03T19:17:42Z

test/gpu/kernels/CMakeLists.txt

+file(GLOB KERNELS_TESTS CONFIGURE_DEPENDS *.cpp)
+list(REMOVE_ITEM KERNELS_TESTS ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp)
+
+message("KERNELS_TESTS: ${KERNELS_TESTS}")


Debug message statements should typically be removed or made conditional before merging to production. Consider removing this message or wrapping it in a conditional check for debug builds.

Suggested change

file(GLOB KERNELS_TESTS CONFIGURE_DEPENDS *.cpp)

list(REMOVE_ITEM KERNELS_TESTS ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp)

message("KERNELS_TESTS: ${KERNELS_TESTS}")

option(MIGRAPHX_VERBOSE "Enable verbose output for kernel tests" OFF)

file(GLOB KERNELS_TESTS CONFIGURE_DEPENDS *.cpp)

list(REMOVE_ITEM KERNELS_TESTS ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp)

if(MIGRAPHX_VERBOSE)

message("KERNELS_TESTS: ${KERNELS_TESTS}")

endif()

CharlieL7

LGTM. How would we test larger kernels like TopK as a unit test?

CharlieL7 · 2025-09-16T22:46:59Z

src/targets/gpu/kernels/include/migraphx/kernels/test.hpp

+{
+    // There is no way to easily exit with no error. We can terminate the
+    // current wavefront without an error, but if there is more wavefronts
+    // than we need to fallback to a trap which throws an error in HSA


Suggested change

// than we need to fallback to a trap which throws an error in HSA

// then we need to fallback to a trap which throws an error in HSA

TedThemistokleous

LGTM to me too. Copilot begin a tad pedantic here but overall lets get this in and ensure we have test coverage

pfultz2 added 16 commits August 1, 2025 18:09

Add gpu kernel unit testing infra

7aa727d

Format

c20742f

Add algorithm tests

b0d7a46

Format

ea12168

Apply fixits

ab3e73a

Format

99250c3

Exclude test.hpp header

bfd58be

Report failures and stop on expect but not on check

35fcec7

Format

e824891

Add parallel compilation

0fd319c

Add array tests

be87e05

Format

25b9bc0

Add shape tests

83729a7

Merge branch 'develop' into gpu-kernel-unit-tests

d943b73

Update standard shape calculation

8995591

Format

c9c886c

pfultz2 requested a review from causten as a code owner September 3, 2025 19:14

pfultz2 self-assigned this Sep 3, 2025

pfultz2 requested review from CharlieL7, Copilot, kahmed10, TedThemistokleous and shivadbhavsar and removed request for causten and CharlieL7 September 3, 2025 19:15

Copilot AI reviewed Sep 3, 2025

View reviewed changes

pfultz2 and others added 3 commits September 3, 2025 22:01

Merge branch 'develop' into gpu-kernel-unit-tests

f23916e

Fix null

57d61b5

Fix tidy warnings

fdbad61

format

7884aaa

CharlieL7 approved these changes Sep 16, 2025

View reviewed changes

TedThemistokleous approved these changes Oct 1, 2025

View reviewed changes

TedThemistokleous added roadmap Tasks to finish for a release Continous Integration Pull request updates parts of continous integration pipeline labels Oct 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add initial unit tests for jit gpu kernels #4278

Add initial unit tests for jit gpu kernels #4278

Uh oh!

pfultz2 commented Sep 3, 2025 •

edited by CharlieL7

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

CharlieL7 left a comment

Uh oh!

CharlieL7 Sep 16, 2025

Uh oh!

TedThemistokleous left a comment

Uh oh!

Uh oh!

-        k.launch(nullptr, options.global, options.local)(test_cases.at(case_name), failures.get());
+        auto it = test_cases.find(case_name);
+        if(it == test_cases.end())
+        {
+            std::cerr << "Test case not found: " << case_name << std::endl;
+            CHECK(false); // Fail the test
+            return;
+        }
+        k.launch(nullptr, options.global, options.local)(it->second, failures.get());

	test_names.push_back(m[1].str());
	test_names.emplace_back(m[1].str());

	if(op != nullptr or *op != '\0')
	if(op != nullptr && *op != '\0')

	// than we need to fallback to a trap which throws an error in HSA
	// then we need to fallback to a trap which throws an error in HSA

Add initial unit tests for jit gpu kernels #4278

Are you sure you want to change the base?

Add initial unit tests for jit gpu kernels #4278

Uh oh!

Conversation

pfultz2 commented Sep 3, 2025 • edited by CharlieL7 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Changelog Category

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

CharlieL7 left a comment

Choose a reason for hiding this comment

Uh oh!

CharlieL7 Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

TedThemistokleous left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pfultz2 commented Sep 3, 2025 •

edited by CharlieL7

Loading