Update CMake infra to run HIP CXX tests using top-level cmake #10

diptorupd · 2025-10-02T21:20:42Z

The PR changes how the HIP C++ tests in libflashinfer/tests/hip are executed.

The CMake infrastructure for the testing is now fully integrated with the project-level CMake.
Since upstream has removed CUDA C++ unit tests, I went ahead and removed the CUDA configuration path from libflashinfer/tests/CMakeLists.txt.
Replaced FetchContent with find_package for discovering the gtest CMake package.
Made AOT kernel source generation contingent on FLASHINFER_BUILD_KERNELS CMake flag and removed the kernel generation dependency on C++ unit tests. The HIP unit tests do not require AOT kernels to be generated.
~~Now all new C++ HIP unit tests can be added using the configure_flashinfer_target function.~~ Tests now are automatically configured once added to the tests/hip directory and require no changes to the CMakeLists.txt
The PR also makes FLASHINFER_ENABLE_HIP the default from FLASHINFER_ENABLE_CUDA.

Usage after the changes.

mkdir build && cd build
cmake -DFLASHINFER_ENABLE_HIP=ON -DFLASHINFER_UNITTESTS=ON  -GNinja ..
ninja build_tests
ctest --progress --output-on-failure -j12

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

cmake/utils/ConfigureTargets.cmake:1

Removing the dispatch_inc dependency without replacement may cause build failures if tests depend on generated dispatch headers.

# cmake-format: off

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

cmake/Options.cmake

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

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

cmake/utils/ConfigureTargets.cmake

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

cmake/utils/ConfigureTargets.cmake:1

Removing the dispatch_inc dependency may cause build failures if targets require generated dispatch headers. Verify that this dependency is handled elsewhere or add conditional logic to preserve it when needed.

# cmake-format: off

libflashinfer/tests/CMakeLists.txt:1

Removing all test configurations eliminates test coverage for core functionality including decode, prefill, cascade, and other critical components. The HIP tests alone may not provide adequate coverage.

# Set global paths and initialize test list

libflashinfer/tests/CMakeLists.txt:1

Removing all test configurations eliminates test coverage for core functionality including decode, prefill, cascade, and other critical components. The HIP tests alone may not provide adequate coverage.

# Set global paths and initialize test list

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

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

cmake/utils/ConfigureTargets.cmake:1

Removing the dispatch_inc dependency for all targets may cause build failures if some targets still require it. Consider making this conditional based on whether kernels are being built.

# cmake-format: off

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

cmake/Options.cmake

Copilot · 2025-10-03T03:49:39Z

cmake/Options.cmake


 # === HIP/ROCm OPTIONS ===
-flashinfer_option(FLASHINFER_ENABLE_HIP "Enable AMD HIP/ROCm backend" OFF)
+flashinfer_option(FLASHINFER_ENABLE_HIP "Enable AMD HIP/ROCm backend" ON)


Changing the default HIP backend from OFF to ON may break existing builds that don't have HIP dependencies installed. Consider keeping the original default or documenting this breaking change.

Suggested change

flashinfer_option(FLASHINFER_ENABLE_HIP "Enable AMD HIP/ROCm backend" ON)

flashinfer_option(FLASHINFER_ENABLE_HIP "Enable AMD HIP/ROCm backend" OFF)

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

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

cmake/utils/ConfigureTargets.cmake

Copilot · 2025-10-03T15:00:56Z

CMakeLists.txt

 # cmake-format: off
 # Compiler flags - defined as lists for cleaner management
 set(WARNING_FLAGS
    "-Wall"


Removing -Wextra reduces compiler warning coverage. This change appears unrelated to the HIP CMake infrastructure update and should be justified or reverted.

Suggested change

"-Wall"

"-Wall"

"-Wextra"

rtmadduri · 2025-10-03T15:13:08Z

Currently the platform team runs tests in a certain way. Even our Dockerfile runs CMake based on the old infra.

If we make changes to the C++ test infra, we need to coordinate the changes with platform team. Right now this Dockerfile is how they release artifacts.

rtmadduri · 2025-10-03T15:31:29Z

I have approved the PR. Lets make a note to change the Dockerfile.rocm_ci

demandal25 · 2025-10-03T17:19:23Z

The following tests are failing

This PR adds chunking logic and enables the shared memory optimization feature for Decode for the CDNA3 architecture. The major addition of the PR is rewriting the shared memory calculation and chunking to better suit the CDNA3 architecture which only allows 64KiB of shared memory per CU. The PR makes corresponding changes to `test_batch_decode_kernels_hip.py` and `examples/test_batch_decode_example.py` `examples/test_batch_decode_example.py` ``` JIT: Using prebuilt ops PASS ``` `test_batch_decode_kernels_hip.py` ``` ================================= 720 passed in 74.37s (0:01:14) ================================= ``` Complete HIP PyTest suite ``` ================================= 16388 passed, 18 skipped in 148.54s (0:02:28) ================================= ``` C++ test suite ``` 89% tests passed, 3 tests failed out of 27 Total Test time (real) = 259.46 sec The following tests FAILED: 3 - FlashInferCorrectnessTest.VariableLengthMergeKernelCorrectnessTestFP16 (Failed) 20 - MfmaRowSumTest.CorrectResults (Failed) 27 - test_rowsum_hip (Failed) ``` Note: See [here](#10 (comment)) for more info about the above known failures Improvement over the existing implementation: ``` num_qo_heads = 32 kv_len = 8196 num_kv_heads = 32 head_dim = 128 num_iter = 500 Average time per iteration in seconds: Current Flashinfer + ROCm Decode MI325: 6.3011 Shared memory Optimization Decode MI325 (This PR): 0.11113595962524414 Upstream Flashinfer v0.2.5 Decode H100: 0.09272098541259766 ```

Copilot AI review requested due to automatic review settings October 2, 2025 21:20

This comment was marked as outdated.

Sign in to view

demandal25 self-requested a review October 2, 2025 22:49

demandal25 approved these changes Oct 2, 2025

View reviewed changes

diptorupd requested a review from rtmadduri October 3, 2025 02:16

diptorupd marked this pull request as draft October 3, 2025 02:39

diptorupd force-pushed the feature/integrate_hip_cxx_tests branch from c93c75a to a7666ec Compare October 3, 2025 03:29

diptorupd requested a review from demandal25 October 3, 2025 03:30

diptorupd marked this pull request as ready for review October 3, 2025 03:30

Copilot AI review requested due to automatic review settings October 3, 2025 03:30

Copilot AI reviewed Oct 3, 2025

View reviewed changes

cmake/Options.cmake Show resolved Hide resolved

diptorupd requested a review from Copilot October 3, 2025 03:32

Copilot AI reviewed Oct 3, 2025

View reviewed changes

cmake/utils/ConfigureTargets.cmake Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings October 3, 2025 03:33

Copilot AI reviewed Oct 3, 2025

View reviewed changes

diptorupd force-pushed the feature/integrate_hip_cxx_tests branch from 268ac40 to a33cad1 Compare October 3, 2025 03:42

diptorupd requested a review from Copilot October 3, 2025 03:49

Copilot AI reviewed Oct 3, 2025

View reviewed changes

demandal25 approved these changes Oct 3, 2025

View reviewed changes

diptorupd added 2 commits October 3, 2025 10:59

Update Cmake infra to run HIP CXX tests using top-level cmake

09b5d86

Remove -Wextra and made test discovery easier

747b12e

Copilot AI review requested due to automatic review settings October 3, 2025 15:00

diptorupd force-pushed the feature/integrate_hip_cxx_tests branch from 60aeed3 to 747b12e Compare October 3, 2025 15:00

Copilot AI reviewed Oct 3, 2025

View reviewed changes

rtmadduri approved these changes Oct 3, 2025

View reviewed changes

diptorupd mentioned this pull request Oct 3, 2025

Update docker/Dockerfile.rocm_ci to point to new instructions for running CXX tests #12

Open

diptorupd merged commit 63291be into amd-integration Oct 3, 2025
1 check passed

diptorupd deleted the feature/integrate_hip_cxx_tests branch October 3, 2025 18:32

rtmadduri mentioned this pull request Oct 21, 2025

Decode feature chunking logic and shared mem optimization #25

Merged

	flashinfer_option(FLASHINFER_ENABLE_HIP "Enable AMD HIP/ROCm backend" ON)
	flashinfer_option(FLASHINFER_ENABLE_HIP "Enable AMD HIP/ROCm backend" OFF)

Update CMake infra to run HIP CXX tests using top-level cmake #10

Update CMake infra to run HIP CXX tests using top-level cmake #10

Uh oh!

Conversation

diptorupd commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

rtmadduri commented Oct 3, 2025

Uh oh!

rtmadduri commented Oct 3, 2025

Uh oh!

demandal25 commented Oct 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

diptorupd commented Oct 2, 2025 •

edited

Loading