make hip-tests compileable with TheRock by cjatin · Pull Request #1624 · ROCm/rocm-systems

cjatin · 2025-10-30T12:39:04Z

Motivation

Make changes through out hip-tests to make sure it compileable with TheRock

Technical Details

Upgrade Catch2
Make language detection changes
Packaging detection changes

Test Plan

Existing tests should pass without any issues

Test Result

running tests

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

jayhawk-commits · 2025-11-04T20:51:18Z

With the amount of files changed, I would feel more confident with SMEs from the hip runtime team reviewing, plus links to successful logs from both the legacy CI systems once the corresponding changes are made there plus logs from TheRock build system, when paired with ROCm/TheRock#2001

ScottTodd · 2025-11-05T21:27:34Z

+if(NOT THEROCK_BUILD_MODE)
+    # Setup catch2 manually
+    include(FetchContent)
+    FetchContent_Declare(
+        Catch2
+        URL "https://rocm-third-party-deps.s3.us-east-2.amazonaws.com/Catch2-3.8.1.tar.gz"
+        DOWNLOAD_EXTRACT_TIMESTAMP OFF
+        URL_HASH SHA256=18b3f70ac80fccc340d8c6ff0f339b2ae64944782f8d2fca2bd705cf47cadb79
+    )
+    FetchContent_MakeAvailable(Catch2)
+else()
+    # TheRock has already setup Catch2 for us
+    find_package(Catch2 REQUIRED)
 endif()


Copying a comment from ROCm/TheRock#2001 (comment)

I don't think a custom flag (especially not one referencing TheRock) should really be needed here. Other subprojects use find_package without such requirements. See https://github.com/ROCm/TheRock/blob/main/docs/development/dependencies.md for best practices. I think this general pattern should work in this case too: https://github.com/ROCm/rocm-libraries/blob/68b549eb4d2ad4a936526a60f79e0bdd91e7ad34/projects/hipblas/cmake/get-rocm-cmake.cmake#L6-L13

find_package(PackageName ...) if(NOT PackageName_FOUND) FetchContent_Declare( ... ) FetchContent_MakeAvailable(...) find_pacakge(PackageName ... PATHS ...) )

made some changes, have a look

Thanks, the changes in projects/hip-tests/catch/CMakeLists.txt look better to me now.

By the way, reviews are easier if you avoid rewriting git history and force pushing. Prefer to use merges instead of history rewriting rebases to sync your branch and push new commits in response to comments. I can't easily see what specifically changed between my prior comment and now:

This force push compare shows unrelated changes:

"Changes since your last review" doesn't understand history rewrites:

jayhawk-commits · 2025-11-26T17:00:02Z

                      TEST_SRC ${TEST_SRC}
-                      TEST_TARGET_NAME build_tests
-                      COMPILE_OPTIONS -std=c++17)
+                      TEST_TARGET_NAME build_tests)


missing new-line at the end here

jayhawk-commits · 2025-11-26T17:00:13Z

                      TEST_SRC ${TEST_SRC}
-                      TEST_TARGET_NAME build_tests
-                      COMPILE_OPTIONS -std=c++17)
+                      TEST_TARGET_NAME build_tests)


missing new-line at the end

jayhawk-commits · 2025-11-26T17:01:42Z

    fp16_ops.cc
    fp8_host.cc
-    fp8_e8m0.cc
+    #fp8_e8m0.cc


Can a comment be added as to why this is now commented out?

Done, added a TODO

jayhawk-commits · 2025-11-26T17:02:23Z

  constexpr size_t num_of_ops = 18;
  constexpr size_t iters = 100;
-  Catch::Generators::RandomFloatingGenerator<float> input1_gen(2.2f, 10.f);
+  Catch::Generators::RandomFloatingGenerator<float> input1_gen(2.2f, 10.f, /*seed*/ 0x1234);


Is the intention to always use the same seed?

At the moment yes, the old catch2 API did not accept any seed. The new one does.

Are there any concerns that there is a gap in testing if it's always the same seed?

I do not think so, but let me check the API of old catch2's interface and see how they handled seed.

jayhawk-commits · 2025-11-26T17:03:26Z

-                      TEST_TARGET_NAME build_tests
-                      COMPILE_OPTIONS -std=c++17)
+                      TEST_TARGET_NAME build_tests)
 target_link_libraries(GLInteropTest OpenGL::GL OpenGL::EGL GLUT::GLUT)


missing new-line here

jayhawk-commits · 2025-11-26T17:05:43Z

    message(STATUS "Doxygen was not found. Building test plan documentation will not be available")
 endif()
+
+


One too many lines here

jayhawk-commits · 2025-11-26T17:06:26Z

+inccmake_files = glob.glob(build_folder + "/**/*"+inc_cmake_pattern, recursive=True)
+make_test_files_portable(inccmake_files)
+
+


extra lines at the end here

jayhawk-commits · 2025-11-26T17:06:45Z

+    #print(f"In relative_paths.py [1]: build folder: {build_folder}")
+    #print(f"In relative_paths.py [2]: install folder: {install_folder}")
+else:
+    print(f"Args not provided. Please provide arg1-build folder-catch_tests, arg2-install folder")


Perhaps providing an example of the expected args would be helpful here.

this script is a workaround script that handles some ctest/catch downsides and is only called within the hip-tests packaging build files. . I have an open item to completely get rid of this file and have a structural fix.

Also, the example is in the comment.
1st arg - build folder catch_tests
2nd arg - install folder the user wishes to install the tests.

jayhawk-commits · 2025-11-26T17:11:31Z

-      "/opt/rocm/bin/rocm_agent_enumerator | awk '$0 != \"gfx000\"' | xargs | sed -e 's/ /;/g' | "
-      "tr -d '\n'";
+  constexpr std::string_view rocm_agent_enum = "rocm_agent_enumerator";
+  std::filesystem::path default_rocm_bin_path = "/opt/rocm";


Can we remove this default path value and still have it work?

No, we need to find rocm_agent_enumerator. We allow user to point to it with ROCM_PATH, if we do not find it, we need a fallback path.

OK. Note that this fallback probably should be removed for ROCm 8.0 onward.

Not sure what you mean here. hip-tests need to find rocm_agent_enum, users can point to it via setting environment variable ROCM_PATH which is what we do in TheRock enablement cmake change. But if user has not defined that variable fallback path is checking /opt/rocm. Are we changing the installation directory of /opt/rocm in ROCm 8.0?

rahulc-gh

skipping windows CI gates based on - https://mlsejenkinsvm.amd.com/job/sandbox/job/hip-rt-psdb/9828/

## Motivation Resolved: SWDEV-566226 The current implementation of agents inside of rocprof-systems keeps just the minimal necessary set of information required for populating the `info_agent` table inside of rocpd database. There is a sufficient amount of data that is being left out from database, so this change should fix that and store the additional agent information as an `extdata` row inside of `info_agent` table. ## Technical Details This PR introduces additional filed inside of `agent` structure inside which is representing the JSON formatted string of all the additional information we can acquire about particular agent. This data is processed and added during the initial fetching of agents, and afterwards pushed inside of the database. [rocm-systems] ROCm/rocm-systems#1624 (commit 8e1aee6)

## Motivation This change enables hip-tests to be built on TheRock. This is the first step in getting hip-tests building/running with TheRock. ## Technical Details Enable hip-tests to be built with TheRock, when user enables testing via flag `THEROCK_BUILD_TESTING`. [hip-tests](https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch) are collection of tests that are used to test the hip-runtime. A corresponding PR is also required in hip-tests to make it compatible with TheRock, which are being done [here](ROCm/rocm-systems#1624). Subsequent PRs are required to run the built hip-tests which will follow after this PR. ROCm/rocm-systems#2187 - PR will fix the paths to be consistent across linux and windows. Below issues will be fixed with it https://github.com/ROCm/TheRock/pull/2373/files#r2582804642 #2373 (comment) #2001 (comment) #2001 (comment) ## Test Plan The build should complete end to end without failures, next set of changes will enable the running of tests. ## Test Result NA ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

This reverts commit 8e1aee6.

## Motivation This change enables hip-tests to be built on TheRock. This is the first step in getting hip-tests building/running with TheRock. ## Technical Details Enable hip-tests to be built with TheRock, when user enables testing via flag `THEROCK_BUILD_TESTING`. [hip-tests](https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch) are collection of tests that are used to test the hip-runtime. A corresponding PR is also required in hip-tests to make it compatible with TheRock, which are being done [here](ROCm/rocm-systems#1624). Subsequent PRs are required to run the built hip-tests which will follow after this PR. ROCm/rocm-systems#2187 - PR will fix the paths to be consistent across linux and windows. Below issues will be fixed with it https://github.com/ROCm/TheRock/pull/2373/files#r2582804642 ROCm#2373 (comment) ROCm#2001 (comment) ROCm#2001 (comment) ## Test Plan The build should complete end to end without failures, next set of changes will enable the running of tests. ## Test Result NA ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

## Motivation Enable hip-tests execution on TheRock linux and windows. Aligns installation path of hip-tests on linux and Windows enables sharding of hip-tests ## Details Enable hip-tests execution on TheRock linux and windows. **This PR fixes -** #1354 **Hip-tests source that gets built/executed as part of this PR** https://github.com/ROCm/rocm-systems/tree/develop/projects/hip-tests/catch **Related PRs to enable build/execution of tests -** #2373 - enables building of tests ROCm/rocm-systems#1624 - prepares rocm-systems to be built with catchlib from upstream , compiler/hip detection, packaging change ROCm/rocm-systems#2187 - aligns installation path for linux and windows **few other script changes to have compatible builds on older build frameworks.** https://github.com/AMD-ROCm-Internal/build-infra/pull/316 https://github.com/AMD-Radeon-Driver/compute-utils/pull/169 https://github.com/AMD-Radeon-Driver/compute-utils/pull/171 hip-tests executed and verified successfully ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. --------- Co-authored-by: Geo Min <geomin12@amd.com>

github-actions Bot added project: clr project: hip-tests labels Oct 30, 2025

systems-assistant Bot added the organization: ROCm label Oct 30, 2025

cjatin force-pushed the users/jatin/TheRock branch from e6f7581 to a37743c Compare October 31, 2025 16:40

cjatin marked this pull request as ready for review November 3, 2025 17:26

cjatin requested review from a team as code owners November 3, 2025 17:26

jayhawk-commits self-requested a review November 4, 2025 20:34

jayhawk-commits mentioned this pull request Nov 4, 2025

Enable hip-tests on TheRock ROCm/TheRock#2001

Merged

1 task done

ScottTodd reviewed Nov 5, 2025

View reviewed changes

cjatin force-pushed the users/jatin/TheRock branch from 5bff44a to b3d4de3 Compare November 6, 2025 16:17

jayhawk-commits added a commit to ROCm/TheRock that referenced this pull request Nov 7, 2025

Temporarily change submodule to ROCm/rocm-systems#1624

d612474

agunashe force-pushed the users/jatin/TheRock branch from ef80c5c to 6199930 Compare November 11, 2025 20:16

cjatin force-pushed the users/jatin/TheRock branch 2 times, most recently from 80af2c7 to 33548c0 Compare November 17, 2025 15:10

agunashe marked this pull request as draft November 19, 2025 01:34

agunashe force-pushed the users/jatin/TheRock branch 2 times, most recently from 87bf9b7 to d7103b9 Compare November 22, 2025 00:21

agunashe marked this pull request as ready for review November 25, 2025 21:41

jayhawk-commits reviewed Nov 26, 2025

View reviewed changes

cjatin requested review from a team and jrmadsen as code owners December 1, 2025 11:07

github-actions Bot added documentation Improvements or additions to documentation project: amdsmi project: rocm-smi-lib project: rocprofiler-sdk project: rocprofiler-systems labels Dec 1, 2025

cjatin and others added 2 commits December 1, 2025 15:12

Review comments

ee2a0c3

Merge branch 'develop' into users/jatin/TheRock

297660e

cjatin mentioned this pull request Dec 2, 2025

Add hip-tests to be built with TheRock ROCm/TheRock#2373

Merged

1 task done

Merge branch 'develop' into users/jatin/TheRock

faf9089

agunashe requested a review from chrispaquot December 2, 2025 21:54

chrispaquot approved these changes Dec 2, 2025

View reviewed changes

agunashe and others added 2 commits December 2, 2025 19:32

skip the 3 failing tests to merge hip-tests rocm-systems PR

7500942

Merge branch 'develop' into users/jatin/TheRock

2d35269

rahulc-gh approved these changes Dec 3, 2025

View reviewed changes

rahulc-gh merged commit 8e1aee6 into develop Dec 3, 2025
21 of 22 checks passed

rahulc-gh deleted the users/jatin/TheRock branch December 3, 2025 16:53

agunashe mentioned this pull request Dec 15, 2025

SWDEV-1 - catchv3 with packaging changes #1594

Closed

1 task

agunashe added a commit that referenced this pull request Dec 19, 2025

Revert "make hip-tests compileable with TheRock (#1624)"

dd3af72

This reverts commit 8e1aee6.

sshlyapn mentioned this pull request Dec 30, 2025

[HIP] Fix double to E8M0 conversion underflow (avoid unsigned exp wraparound) #2461

Merged

1 task done

		message(STATUS "Doxygen was not found. Building test plan documentation will not be available")
		endif()

		inccmake_files = glob.glob(build_folder + "/*/"+inc_cmake_pattern, recursive=True)
		make_test_files_portable(inccmake_files)

Conversation

cjatin commented Oct 30, 2025

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Uh oh!

jayhawk-commits commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulc-gh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants