Replace SUCCEED-based skips with HipTest::HIP_SKIP_TEST in hip-tests#4567
Open
Replace SUCCEED-based skips with HipTest::HIP_SKIP_TEST in hip-tests#4567
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates HIP Catch2 tests to report runtime skips as skipped (via HipTest::HIP_SKIP_TEST) instead of passed (via SUCCEED), so ctest can correctly classify these cases using SKIP_REGULAR_EXPRESSION "HIP_SKIP_THIS_TEST".
Changes:
- Replace
SUCCEED-based runtime skip paths (multi-GPU/P2P/mempool/managed memory/arch/no-device/etc.) withHipTest::HIP_SKIP_TEST. - Add some early
return/continuecontrol-flow fixes so skip paths don’t proceed into unsupported setup/execution. - Fix a few skip-message formatting cases that previously relied on stream insertion by building strings explicitly.
Reviewed changes
Copilot reviewed 90 out of 90 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/hip-tests/catch/unit/texture/hipTexRefSetGetMipmappedArray.cc | Use HIP_SKIP_TEST when mipmapped arrays aren’t supported. |
| projects/hip-tests/catch/unit/texture/hipBindTextureToMipmappedArray.cc | Use HIP_SKIP_TEST for unsupported mipmap platform/config. |
| projects/hip-tests/catch/unit/streamperthread/hipStreamPerThrdTsts.cc | Convert managed-mem / coop-launch skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/stream/hipMultiStream.cc | Skip when no GPU is available (now a real ctest skip). |
| projects/hip-tests/catch/unit/stream/hipLaunchHostFunc.cc | Skip multi-device test when devices < 2. |
| projects/hip-tests/catch/unit/occupancy/hipOccupancyMaxPotentialBlockSizeVariableSMemWithFlags.cc | Skip MGPU test when devices < 2. |
| projects/hip-tests/catch/unit/module/hip_module_launch_kernel_common.hh | Convert MGPU-dependent section skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/module/hipModuleGetGlobal.cc | Skip when devices < 2. |
| projects/hip-tests/catch/unit/module/hipModuleGetFunction.cc | Skip when devices < 2. |
| projects/hip-tests/catch/unit/memory/mempool_common.hh | Skip when memory pools aren’t supported. |
| projects/hip-tests/catch/unit/memory/hipPointerGetAttribute.cc | Convert P2P/multi-device skips to HIP_SKIP_TEST and add a return path. |
| projects/hip-tests/catch/unit/memory/hipMemcpy_EdgeCases.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early return in one path. |
| projects/hip-tests/catch/unit/memory/hipMemcpyWithStream_old.cc | Convert single-GPU skips to HIP_SKIP_TEST and restructure for early return. |
| projects/hip-tests/catch/unit/memory/hipMemcpyPeerAsync_old.cc | Convert skip conditions to HIP_SKIP_TEST and add early return. |
| projects/hip-tests/catch/unit/memory/hipMemcpyParam2DAsync_old.cc | Convert peer-access / device-count skips to HIP_SKIP_TEST and add return for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpyHtoA_old.cc | Convert peer-access / device-count skips to HIP_SKIP_TEST and add return for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpyDtoDAsync.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST with early returns. |
| projects/hip-tests/catch/unit/memory/hipMemcpyAtoH_old.cc | Convert peer-access / device-count skips to HIP_SKIP_TEST and add return for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpyAsync_old.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early return in one path. |
| projects/hip-tests/catch/unit/memory/hipMemcpy3D_old.cc | Convert peer-access / device-count skips to HIP_SKIP_TEST and add return for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpy3DAsync_old.cc | Convert peer-access / device-count skips to HIP_SKIP_TEST and add returns for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpy2DToArray_old.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early returns for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpy2DToArrayAsync_old.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early returns for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpy2DFromArray_old.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early returns for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpy2DFromArrayAsync_old.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early returns for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemcpy2DAsync_old.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early return for device-count. |
| projects/hip-tests/catch/unit/memory/hipMemVmm.cc | Convert VMM-support skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipMemRangeGetAttribute_old.cc | Convert managed-memory / pageable-HMM stability skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipMemPrefetchAsyncExtTsts.cc | Convert managed-memory / multi-GPU skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipMemPoolTrimTo.cc | Convert MGPU skip to HIP_SKIP_TEST and return early. |
| projects/hip-tests/catch/unit/memory/hipMemPoolApi.cc | Convert memory-pool support skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipMemCoherencyTst.cc | Convert managed-memory capability skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipMemAdvise_old.cc | Convert MGPU / managed-memory skips to HIP_SKIP_TEST and add early returns in some paths. |
| projects/hip-tests/catch/unit/memory/hipMemAdviseMmap.cc | Convert pageable-memory-access skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipMallocManaged_MultiScenario.cc | Convert pageable-HMM oversubscription stability skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipMallocManaged.cc | Convert stream-insertion skip messages to formatted strings + HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/memory/hipMallocArray.cc | Convert gfx-specific skip to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/memory/hipHostMalloc.cc | Convert host-mapping/coherence skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipHostAlloc.cc | Convert host-mapping skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/memory/hipFreeMipmappedArray.cc | Convert “insufficient memory” skip to HIP_SKIP_TEST with formatted message; add <string>. |
| projects/hip-tests/catch/unit/memory/hipDrvMemcpy3D_old.cc | Convert MGPU device-count skips to HIP_SKIP_TEST + early returns. |
| projects/hip-tests/catch/unit/memory/hipDrvMemcpy3DAsync_old.cc | Convert MGPU device-count skips to HIP_SKIP_TEST + early returns. |
| projects/hip-tests/catch/unit/graph/hipStreamBeginCapture_old.cc | Convert MGPU skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/graph/hipStreamBeginCapture.cc | Convert MGPU skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/graph/hipGraphUpload.cc | Convert no-device skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/graph/hipGraphMemAllocNodeGetParams.cc | Convert no-device skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/graph/hipGraphLaunch_old.cc | Convert no-device skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/graph/hipGraphInstantiateWithFlags.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add returns for device-count. |
| projects/hip-tests/catch/unit/graph/hipGraphCloneComplx.cc | Convert MGPU skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/graph/hipGraphClone.cc | Convert MGPU/P2P skip paths to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/graph/hipGraphAddMemcpyNodeToSymbol_old.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early returns. |
| projects/hip-tests/catch/unit/graph/hipGraphAddMemcpyNodeFromSymbol_old.cc | Convert MGPU/P2P skips to HIP_SKIP_TEST and add early returns. |
| projects/hip-tests/catch/unit/graph/hipGraphAddChildGraphNode.cc | Convert MGPU skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/graph/hipDrvGraphAddMemcpyNode.cc | Convert MGPU skip to HIP_SKIP_TEST and add early return. |
| projects/hip-tests/catch/unit/executionControl/hipLaunchCooperativeKernelMultiDevice.cc | Convert device-count skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/event/Unit_hipEventMGpuMThreads.cc | Convert MGPU skips to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/deviceLib/unsafeAtomicAdd_RTC.cc | Convert feature/host-mapping skips to HIP_SKIP_TEST; add <string> and formatted messages. |
| projects/hip-tests/catch/unit/deviceLib/unsafeAtomicAdd_NonCoherent_withunsafeflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/unsafeAtomicAdd_NonCoherent_withoutflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/unsafeAtomicAdd_NonCoherent_withnounsafeflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/unsafeAtomicAdd_Coherent_withunsafeflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/unsafeAtomicAdd_Coherent_withoutflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/unsafeAtomicAdd_Coherent_withnounsafeflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/threadfence_system.cc | Convert allocation-failure “skip” to HIP_SKIP_TEST. |
| projects/hip-tests/catch/unit/deviceLib/BuiltIns_fmin.cc | Convert feature/host-mapping skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/BuiltIns_fmax.cc | Convert feature/host-mapping skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/AtomicAdd_NonCoherent_withunsafeflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/AtomicAdd_NonCoherent_withoutflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/AtomicAdd_NonCoherent_withnoUnsafeflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/AtomicAdd_Coherent_withunsafeflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/AtomicAdd_Coherent_withoutflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/unit/deviceLib/AtomicAdd_Coherent_withnoUnsafeflag.cc | Convert skips to HIP_SKIP_TEST; add <string>. |
| projects/hip-tests/catch/stress/memory/hipMemPrftchAsyncStressTst.cc | Convert managed-memory skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/stress/memory/hipMallocManagedStress.cc | Convert managed-memory skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/perftests/vmm/hipPerfVMMAlloc.cc | Convert no-device perf skip to HIP_SKIP_TEST; adjust message. |
| projects/hip-tests/catch/perftests/memory/hipPerfSharedMemReadSpeed.cc | Convert no-device perf skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/perftests/memory/hipPerfMemcpy.cc | Convert no-device perf skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/perftests/memory/hipPerfMemMallocCpyFree.cc | Convert no-device perf skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/perftests/memory/hipPerfHostNumaAlloc.cc | Convert invalid device-count skip to HIP_SKIP_TEST and return early. |
| projects/hip-tests/catch/perftests/memory/hipPerfDevMemWriteSpeed.cc | Convert no-device perf skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/perftests/memory/hipPerfDevMemReadSpeed.cc | Convert no-device perf skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/perftests/memory/hipPerfBufferCopySpeedAll2All.cc | Replace trailing SUCCEED with REQUIRE(true). |
| projects/hip-tests/catch/perftests/memory/hipPerfBufferCopySpeed.cc | Convert no-device perf skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/perftests/memory/hipPerfBufferCopyRectSpeed.cc | Convert no-device perf skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/multiproc/hipGetDevicePropertiesMproc.cc | Convert “not enough GPUs” skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/multiproc/hipGetDeviceAttributeMproc.cc | Convert “not enough GPUs” skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/multiproc/hipDeviceTotalMemMproc.cc | Convert “not enough GPUs” skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/multiproc/hipDeviceGetPCIBusIdMproc.cc | Convert “not enough GPUs” skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/multiproc/hipDeviceComputeCapabilityMproc.cc | Convert “not enough GPUs” skip to HIP_SKIP_TEST. |
| projects/hip-tests/catch/include/hip_test_checkers.hh | Convert missing-assembly-file “skip” to HIP_SKIP_TEST. |
Comments suppressed due to low confidence (1)
projects/hip-tests/catch/unit/deviceLib/threadfence_system.cc:49
- If hipHostMalloc(&data, ...) fails, HIP_SKIP_TEST only prints and execution continues;
datais uninitialized and is dereferenced on the next line. After printing the skip message, return early (and only free resources that were successfully allocated) to avoid undefined behavior/crashes.
if (hipHostMalloc(&data, sizeof(int), hipHostMallocCoherent) != hipSuccess) {
HipTest::HIP_SKIP_TEST("Memory allocation failed. Skip test. Is SVM atomic supported?");
}
constexpr int init_data = 1000;
*data = init_data;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/hip-tests/catch/unit/memory/hipMemcpyParam2DAsync_old.cc
Outdated
Show resolved
Hide resolved
sruscica
reviewed
Mar 30, 2026
projects/hip-tests/catch/perftests/memory/hipPerfBufferCopyRectSpeed.cc
Outdated
Show resolved
Hide resolved
sruscica
reviewed
Mar 30, 2026
projects/hip-tests/catch/perftests/memory/hipPerfBufferCopySpeedAll2All.cc
Outdated
Show resolved
Hide resolved
sruscica
reviewed
Mar 31, 2026
sruscica
reviewed
Mar 31, 2026
projects/hip-tests/catch/unit/memory/hipMallocManaged_MultiScenario.cc
Outdated
Show resolved
Hide resolved
sruscica
reviewed
Mar 31, 2026
sruscica
reviewed
Mar 31, 2026
sruscica
reviewed
Mar 31, 2026
sruscica
reviewed
Mar 31, 2026
sruscica
reviewed
Mar 31, 2026
projects/hip-tests/catch/unit/memory/hipMemPrefetchAsyncExtTsts.cc
Outdated
Show resolved
Hide resolved
sruscica
reviewed
Mar 31, 2026
projects/hip-tests/catch/unit/memory/hipMemPrefetchAsyncExtTsts.cc
Outdated
Show resolved
Hide resolved
iassiour
force-pushed
the
fix/hip-tests-use-hip-skip-test-instead-of-succeed
branch
from
34beae3 to
870a4dd
Compare
Runtime skips (multi-GPU, P2P, memory pool, managed memory, arch checks, perf no-device, etc.) previously used Catch SUCCEED, which reports pass. Use HipTest::HIP_SKIP_TEST so ctest can detect HIP_SKIP_THIS_TEST. Includes control-flow fixes where skip paths must return before further setup, and string formatting where skips used stream insertion. Made-with: Cursor
- threadfence_system: return after HIP_SKIP_TEST; free first allocation if second fails - hipMallocManaged Advanced: free managed buffers (and events when created) before return on skip - hipMemcpyParam2DAsync_old: free pitch + host buffers when skipping for no P2P - hipMemcpy3D_old: single-line HIP_SKIP_TEST strings (fix invalid literals) - hipPerfBufferCopyRectSpeed: one-line skip message (reviewer request) - hipPerfBufferCopySpeedAll2All: CHECK(true) + comment vs bare REQUIRE(true) Made-with: Cursor
Introduce HipTest::SkipReason constexpr messages in hip_test_common.hh so duplicate skip conditions share one stable string for logs and ctest. Replace matching HIP_SKIP_TEST string literals across catch tests (P2P, multi-GPU, memory pool, managed memory, host pinned, VMM, cooperative launch, PCIe atomics, warp intrinsics, etc.). Wire CHECK_PCIE_ATOMIC_SUPPORT to kPcieAtomicUnsupported. Add tools/apply_skip_reason_constants.py to document and reproduce the literal-to-constant mapping. Made-with: Cursor
hipSetValidDevices and hipGetDeviceCount skip when deviceCount < 2, i.e. they require at least two GPUs. Replace mistaken kFewerThanThreeGpus and align skip messages with HipTest::SkipReason::kFewerThanTwoGpus. Remove unused kFewerThanThreeGpus and fix apply_skip_reason_constants.py mapping. Made-with: Cursor
Drop the one-off bulk-replacement script from the tree; skip reason constants remain in hip_test_common.hh. Made-with: Cursor
…dMemory skips Replace multi-line HIP_SKIP_TEST strings about GPU 0 lacking managed memory with HipTest::SkipReason::kManagedMemoryUnsupported in stress and unit tests. Made-with: Cursor
Add SkipReason::kFineGrainHwUnsupported for stable skip messages. cache_coherency_cpu_gpu: gate device fine-grained path on CheckIfFeatSupported(CT_FEATURE_FINEGRAIN_HWSUPPORT) instead of hipDeviceAttributeFineGrainSupport. unsafeAtomicAddDevice: emit HIP_SKIP_TEST when fine-grain CT feature is unsupported (previously vacuous pass). AtomicAdd / unsafeAtomicAdd / RTC tests: replace ad-hoc skip_gfx_msg else branches with the canonical skip reason. Includes hipEventCreateWithFlags updates on this branch. Made-with: Cursor
iassiour
force-pushed
the
fix/hip-tests-use-hip-skip-test-instead-of-succeed
branch
from
a6f2356 to
74acd4b
Compare
sruscica
reviewed
Apr 1, 2026
| /* Test whether target device supports cooperative groups ****************/ | ||
| if (device_properties.cooperativeLaunch == 0) { | ||
| SUCCEED("Cooperative group support not available..."); | ||
| HipTest::HIP_SKIP_TEST("Cooperative group support not available..."); |
Contributor
There was a problem hiding this comment.
kCooperativeLaunchUnsupported
sruscica
reviewed
Apr 1, 2026
| HipTest::HIP_SKIP_TEST(HipTest::SkipReason::kFineGrainHwUnsupported); | ||
| return true; | ||
| } | ||
| deviceFineGrain = 1; |
sruscica
reviewed
Apr 1, 2026
| SUCCEED( | ||
| HipTest::HIP_SKIP_TEST( | ||
| "Mipmaps are Supported only on windows on devices with image support," | ||
| " skipping the test."); |
Contributor
There was a problem hiding this comment.
Rework to remove skipping the test.
Contributor
There was a problem hiding this comment.
Maybe just kMipmappedArraysUnsupported?
iassiour
force-pushed
the
fix/hip-tests-use-hip-skip-test-instead-of-succeed
branch
from
0135e81 to
c07dc7b
Compare
iassiour
removed request for
a team,
cfreeamd,
dayatsin-amd,
jrmadsen and
kentrussell
iassiour
removed
project: amdsmi
project: rocprofiler-sdk
project: rocprofiler-systems
project: rdc
github actions
iassiour
force-pushed
the
fix/hip-tests-use-hip-skip-test-instead-of-succeed
branch
from
f18003c to
cdca2f1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Runtime skips (multi-GPU, P2P, memory pool, managed memory, arch checks, perf no-device, etc.) previously used Catch SUCCEED, which reports pass. Use HipTest::HIP_SKIP_TEST so ctest can detect HIP_SKIP_THIS_TEST.
Includes control-flow fixes where skip paths must return before further setup, and string formatting where skips used stream insertion.
Made-with: Cursor
Motivation
Technical Details
JIRA ID
Test Plan
Test Result
Submission Checklist