Repository navigation
test(rocjitsu): batch floating-point mode tests - #12833
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
a7cc536 to
9c0fc52
Compare
|
Thanks for doing this! LGTM after Jakub's comment. |
|
Two small suggestions from our local validation of the same FP matrix:
These checks reused our existing FP binary with the same |
CTest currently runs seven probe test bodies for five unique cases: the unfiltered probe registration runs all five, then two filtered registrations repeat cases. Two scalar SCC tests also share a name, so each of their CTest entries runs both bodies. Discover each probe case once and give the raw-encoding scalar test a distinct name. This reduces executions across these registrations from eleven to seven while preserving every test body, assertion, and independent opcode check. Include the generated probe fragment in installed CTest registration to retain the same five cases after installation. Floating-point test batching is handled separately by #12833.
Move the floating-point matrices into a dedicated executable so other tests do not repeatedly register them. Run all FP cases in 16 GoogleTest shards in both build-tree and installed CTest runs, retaining per-case XML results and fresh test fixtures.
Clear each shard XML before starting GoogleTest and fail if cleanup or execution fails. Use the same launcher for build-tree and installed CTest registrations. Pass explicit all-tests and brief flags so inherited shell filters cannot reduce coverage and passing cases do not flood logs.
CMake existence checks and forced removal can treat an inaccessible path as missing. Create an absent report first, then require removal without -f to succeed before starting GoogleTest. Clarify the cleanup guarantee and put the sharding explanation beside its registration loop.
9c0fc52 to
885bc6e
Compare
|
Hi @kuhar can you please take another look? The top 2 commits hopefully address the issues you flagged |
|
Cancelling the remaining gfx950 jobs on this run so the TheRock submodule bump PRs can be prioritized on the gfx950 queue. This change does not target gfx950, and gfx950 was the only work still outstanding. Jobs that already finished are unchanged. Re-run failed jobs if you need gfx950 results before merge. |
The gfx950 jobs shouldn't be anyway running on this PR after ROCm/TheRock#8670 . Would be good to verify. |
TheRock Submodule Bump ActivityNewest first
|
rocjitsu's floating-point tests check lots of combinations of instructions, architectures, operands and mode settings. Each combination is a GoogleTest case, and CTest currently starts a fresh `rocjitsu_tests` process for each one. Every process builds and registers the whole parameter matrix before GoogleTest applies the filter for the one case it needs to run. So adding cases makes every launch more expensive, including launches for unrelated tests. This showed up in [the TheRock emulation timing discussion](ROCm/TheRock#8670 (comment)): the emulation test step grew from 13m17s to 62m41s after the FP matrices expanded. Most of the extra time was going into repeated startup work. This change puts `valu_fp_mode_test.cpp` in its own executable, `rocjitsu_fp_mode_tests`, and runs it in 16 GoogleTest shards (batches). Each batch still registers the full FP matrix, but that now happens 16 times instead of once per case. The other tests no longer register the FP matrix at all. The parameter grids, assertions and fresh emulator setup for each case stay the same. GoogleTest assigns every case to one batch, so new cases are picked up automatically. The 16 batches follow CTest's usual parallelism setting; they also work with a single worker. If you build everything and run `ctest`, the workflow stays the same. Both the build tree and installed tests include the new executable and all 16 batches. To run just the FP tests: ```bash ctest --test-dir build -L '^fp-mode$' --output-on-failure ``` If you use `ctest -R` to select an individual FP case or family, those names are now inside the batches. Use the new executable with a GoogleTest filter instead; for example: ```bash build/tests/rocjitsu_fp_mode_tests --gtest_filter='BinaryF32/ValuFpModeTest.*' ``` Scripts that build or run only `rocjitsu_tests` also need to include `rocjitsu_fp_mode_tests` to keep running the FP coverage. Each batch uses a small CMake launcher that removes its previous XML before starting GoogleTest and fails the CTest batch if cleanup fails. The launcher passes `--gtest_filter=*` so an inherited `GTEST_FILTER` cannot silently reduce coverage, and `--gtest_brief=1` to suppress passing-case chatter while retaining failure diagnostics and per-case XML. Direct executable invocations can still use custom filters. CTest reports `ValuFpModeTest.Shard0` through `Shard15`, so its test count drops even though the same cases run. `ctest --rerun-failed` reruns a failed batch. GoogleTest writes per-case XML under `build/tests/gtest-results/` (or `gtest-results/` in the installed test directory) when it finishes the batch. Ordinary assertion failures let the batch continue; a crash or timeout stops the remaining cases. If the launcher cannot start or complete cleanup, an older report can remain, so read the XML alongside CTest's status. The README documents the selection commands and result locations. The controlled [before](https://github.com/ROCm/rocm-systems/actions/runs/37373531144/job/111976311983) and [after](https://github.com/ROCm/rocm-systems/actions/runs/37373534705/job/111976336177) CI runs measured the original batching change at `a7cc536f7f15`, before the launcher follow-up and subsequent rebases. They used the same TheRock revision, container and two-CPU runner class, with one CTest worker: | Measurement | Before | After | |---|---:|---:| | Entire emulation test step | 67m27s | 14m36s | | rocjitsu CTest wall time | 3867.92 s | 695.79 s | | FP cases/batches, sum of reported durations | 2252.58 s | 3.23 s | | mirage CTest wall time | 178.46 s | 179.27 s | That saves 52m51s, about 78% of the emulation test step, in this paired run. These timings exclude compilation and artifact handling. The 67m27s baseline is from this experiment; the earlier 62m41s is the motivating historical run. All 16 FP batches passed. Both runs still have the same 223 failing non-FP tests, with identical names and failure statuses. Those tests are non-blocking in TheRock, so the successful job results don't mean the whole suite passes. The four regular rocjitsu CI configurations (Release, ASan/UBSan, TSan and GCC UBSan) also passed before the launcher follow-up. Initial local validation of the batching change with Clang 23.1 in Release mode preserved the original 37,795 GoogleTest cases exactly as 5,911 main cases plus 31,884 FP cases. Those are counts from that revision. The batch XML confirmed every FP case passed exactly once, both in the build tree and after relocating the installation. Three shuffled repetitions passed too. Main-executable startup fell from 72.9 ms to 10.4 ms locally; the build and install passed. The launcher follow-up was checked with freshly generated build-tree and installed CTest commands using the retained FP binary with unchanged test source. All 31,884 FP cases ran exactly once despite an unmatched inherited filter, including after relocating the installed tests to a path containing spaces. Focused probes checked deletion errors, symlinks, crashes, assertion failures, timeouts and failed-batch reruns; permission checks ran on CMake 3.28 and 4.4. Formatting and whitespace checks pass. A full local rebuild was not repeated for the launcher follow-up or final rebase. ISSUE ID: ROCm/TheRock#8670 (comment) - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
rocjitsu's floating-point tests check lots of combinations of instructions, architectures, operands and mode settings. Each combination is a GoogleTest case, and CTest currently starts a fresh
rocjitsu_testsprocess for each one. Every process builds and registers the whole parameter matrix before GoogleTest applies the filter for the one case it needs to run. So adding cases makes every launch more expensive, including launches for unrelated tests.This showed up in the TheRock emulation timing discussion: the emulation test step grew from 13m17s to 62m41s after the FP matrices expanded. Most of the extra time was going into repeated startup work.
This change puts
valu_fp_mode_test.cppin its own executable,rocjitsu_fp_mode_tests, and runs it in 16 GoogleTest shards (batches). Each batch still registers the full FP matrix, but that now happens 16 times instead of once per case. The other tests no longer register the FP matrix at all. The parameter grids, assertions and fresh emulator setup for each case stay the same. GoogleTest assigns every case to one batch, so new cases are picked up automatically. The 16 batches follow CTest's usual parallelism setting; they also work with a single worker.If you build everything and run
ctest, the workflow stays the same. Both the build tree and installed tests include the new executable and all 16 batches. To run just the FP tests:ctest --test-dir build -L '^fp-mode$' --output-on-failureIf you use
ctest -Rto select an individual FP case or family, those names are now inside the batches. Use the new executable with a GoogleTest filter instead; for example:build/tests/rocjitsu_fp_mode_tests --gtest_filter='BinaryF32/ValuFpModeTest.*'Scripts that build or run only
rocjitsu_testsalso need to includerocjitsu_fp_mode_teststo keep running the FP coverage.Each batch uses a small CMake launcher that removes its previous XML before starting GoogleTest and fails the CTest batch if cleanup fails. The launcher passes
--gtest_filter=*so an inheritedGTEST_FILTERcannot silently reduce coverage, and--gtest_brief=1to suppress passing-case chatter while retaining failure diagnostics and per-case XML. Direct executable invocations can still use custom filters.CTest reports
ValuFpModeTest.Shard0throughShard15, so its test count drops even though the same cases run.ctest --rerun-failedreruns a failed batch. GoogleTest writes per-case XML underbuild/tests/gtest-results/(orgtest-results/in the installed test directory) when it finishes the batch. Ordinary assertion failures let the batch continue; a crash or timeout stops the remaining cases. If the launcher cannot start or complete cleanup, an older report can remain, so read the XML alongside CTest's status. The README documents the selection commands and result locations.The controlled before and after CI runs measured the original batching change at
a7cc536f7f15, before the launcher follow-up and subsequent rebases. They used the same TheRock revision, container and two-CPU runner class, with one CTest worker:That saves 52m51s, about 78% of the emulation test step, in this paired run. These timings exclude compilation and artifact handling. The 67m27s baseline is from this experiment; the earlier 62m41s is the motivating historical run.
All 16 FP batches passed. Both runs still have the same 223 failing non-FP tests, with identical names and failure statuses. Those tests are non-blocking in TheRock, so the successful job results don't mean the whole suite passes. The four regular rocjitsu CI configurations (Release, ASan/UBSan, TSan and GCC UBSan) also passed before the launcher follow-up.
Initial local validation of the batching change with Clang 23.1 in Release mode preserved the original 37,795 GoogleTest cases exactly as 5,911 main cases plus 31,884 FP cases. Those are counts from that revision. The batch XML confirmed every FP case passed exactly once, both in the build tree and after relocating the installation. Three shuffled repetitions passed too. Main-executable startup fell from 72.9 ms to 10.4 ms locally; the build and install passed.
The launcher follow-up was checked with freshly generated build-tree and installed CTest commands using the retained FP binary with unchanged test source. All 31,884 FP cases ran exactly once despite an unmatched inherited filter, including after relocating the installed tests to a path containing spaces. Focused probes checked deletion errors, symlinks, crashes, assertion failures, timeouts and failed-batch reruns; permission checks ran on CMake 3.28 and 4.4. Formatting and whitespace checks pass. A full local rebuild was not repeated for the launcher follow-up or final rebase.
ISSUE ID: ROCm/TheRock#8670 (comment)