Skip to content

[AMD] [Not-Merge] Move getSMVersion to runtime.cuh and fix ROCm compile (dedup with get_cc_major) - #31141

Closed
kangwangamd wants to merge 4 commits into
sgl-project:mainfrom
kangwangamd:amd/guard-getsmversion-rocm
Closed

[AMD] [Not-Merge] Move getSMVersion to runtime.cuh and fix ROCm compile (dedup with get_cc_major)#31141
kangwangamd wants to merge 4 commits into
sgl-project:mainfrom
kangwangamd:amd/guard-getsmversion-rocm

Conversation

@kangwangamd

@kangwangamd kangwangamd commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Motivation

getSMVersion() (added by #30438, "Delete CUTLASS FP8 blockwise… move SM120 to JIT + SwapAB") lives in python/sglang/jit_kernel/include/sgl_kernel/utils.cuh and calls:

cudaDeviceGetAttribute(&sm_major, cudaDevAttrComputeCapabilityMajor, device_id);
cudaDeviceGetAttribute(&sm_minor, cudaDevAttrComputeCapabilityMinor, device_id);

cudaDevAttrComputeCapabilityMajor/Minor are CUDA-only enums (SM / compute-capability is an NVIDIA concept) and do not exist under HIP. Because utils.cuh is an inline header included transitively by nearly every JIT translation unit, this fails to compile in every gfx950 (MI355X) JIT TU:

error: use of undeclared identifier 'cudaDevAttrComputeCapabilityMajor'
error: use of undeclared identifier 'cudaDevAttrComputeCapabilityMinor'
2 errors generated when compiling for gfx950.

This breaks JIT compilation of all DeepSeek-V4 kernels on gfx950, turning 8/9 MI355X-disaggregation benchmarks red (regression since #30438 landed; last-passing 4cec9ef9d, first-failing merge 7431f35fd).

Modifications

Guard getSMVersion() with #ifndef USE_ROCM, matching the other CUDA-only blocks already guarded in this same file. getSMVersion has no ROCm-JIT caller — its only reference in the JIT tree is its own definition, and the SM120 arch-gating is done in Python (fp8_blockwise_gemm.pyis_sm120_supported()), so guarding the whole function out on ROCm is safe. No CUDA-path change.

Validation

Compile-verified with hipcc -DUSE_ROCM (compile-only; no GPU needed since this is a compile-time break):

arch stock (unguarded) this PR (guarded)
gfx950 (MI355X) — the failing arch, HIP 7.2 undeclared identifier cudaDevAttrComputeCapability* → "2 errors compiling for gfx950" ✅ compiles clean (exit 0)
gfx942 (MI300X), HIP 7.2 ❌ same errors ✅ compiles clean (exit 0)

cc @b8zhong (author of #30438) @BBuf — please take a look. Also cc @AMD-yanfeiwang @kkHuang-amd for ROCm review.


CI States

Latest PR Test (Base): ✅ Run #29559518750
Latest PR Test (Extra): ❌ Run #29559518679

getSMVersion() (added by sgl-project#30438) calls cudaDeviceGetAttribute with the
CUDA-only cudaDevAttrComputeCapabilityMajor/Minor enums. These do not
exist under HIP, and since utils.cuh is an inline header included by
nearly every JIT translation unit, this breaks JIT compilation of all
DeepSeek-V4 kernels on gfx950 (MI355X) -- 8/9 mi355x-disagg benchmarks
red. SM/compute-capability version is a CUDA-only concept with no
ROCm-JIT caller (SM120 gating is done in Python), so guard the whole
function with #ifndef USE_ROCM, matching the other guarded blocks in
this file. No CUDA-path change.

Compile-verified (hipcc -DUSE_ROCM): stock fails on gfx950 and gfx942
with 'use of undeclared identifier cudaDevAttrComputeCapabilityMajor/
Minor'; guarded compiles clean on both.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an #ifndef USE_ROCM preprocessor guard around the getSMVersion function in utils.cuh. This change prevents compilation failures on ROCm platforms (such as gfx950) where CUDA-specific compute-capability attributes are not supported. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@DarkSharpness

Copy link
Copy Markdown
Collaborator

Actually getSMVersion should be moved out of utils.cuh into runtime.cuh (it's a CUDA runtime call) (you may need to change the use-site)

/// \file runtime.cuh
/// \brief Host-side CUDA runtime query helpers.
///
/// Thin wrappers around CUDA occupancy and device-property APIs with
/// automatic error checking via `RuntimeDeviceCheck`.
#pragma once
#include <sgl_kernel/utils.cuh>
#include <cstddef>
#include <cstdint>
#ifndef USE_ROCM
#include <cuda_runtime.h>
#else
#include <hip/hip_runtime.h>
#ifndef cudaOccupancyMaxActiveBlocksPerMultiprocessor
#define cudaOccupancyMaxActiveBlocksPerMultiprocessor hipOccupancyMaxActiveBlocksPerMultiprocessor
#endif
#ifndef cudaDeviceGetAttribute
#define cudaDeviceGetAttribute hipDeviceGetAttribute
#endif
#ifndef cudaDevAttrMultiProcessorCount
#define cudaDevAttrMultiProcessorCount hipDeviceAttributeMultiprocessorCount
#endif
#ifndef cudaDevAttrComputeCapabilityMajor
#define cudaDevAttrComputeCapabilityMajor hipDeviceAttributeComputeCapabilityMajor
#endif
#ifndef cudaRuntimeGetVersion
#define cudaRuntimeGetVersion hipRuntimeGetVersion
#endif
#ifndef cudaOccupancyAvailableDynamicSMemPerBlock
inline hipError_t
cudaOccupancyAvailableDynamicSMemPerBlock(std::size_t* smem, const void* func, int num_blocks, int block_size) {
// HIP does not expose this directly; return max shared mem as conservative estimate
hipDeviceProp_t prop;
int device;
hipGetDevice(&device);
hipGetDeviceProperties(&prop, device);
*smem = prop.sharedMemPerBlock;
return hipSuccess;
}
#endif
#endif
namespace host::runtime {
// Return the maximum number of active blocks per SM for the given kernel
template <typename T>
inline auto get_blocks_per_sm(T&& kernel, int32_t block_dim, std::size_t dynamic_smem = 0) -> uint32_t {
int num_blocks_per_sm = 0;
RuntimeDeviceCheck(
cudaOccupancyMaxActiveBlocksPerMultiprocessor(&num_blocks_per_sm, kernel, block_dim, dynamic_smem));
return static_cast<uint32_t>(num_blocks_per_sm);
}
// Return the number of SMs for the given device
inline auto get_sm_count(int device_id) -> uint32_t {
int sm_count;
RuntimeDeviceCheck(cudaDeviceGetAttribute(&sm_count, cudaDevAttrMultiProcessorCount, device_id));
return static_cast<uint32_t>(sm_count);
}
// Return the Major compute capability for the given device
inline auto get_cc_major(int device_id) -> int {
int cc_major;
RuntimeDeviceCheck(cudaDeviceGetAttribute(&cc_major, cudaDevAttrComputeCapabilityMajor, device_id));
return cc_major;
}
// Return the runtime version
inline auto get_runtime_version() -> int {
int runtime_version;
RuntimeDeviceCheck(cudaRuntimeGetVersion(&runtime_version));
return runtime_version;
}
// Return the maximum dynamic shared memory per block for the given kernel
template <typename T>
inline auto get_available_dynamic_smem_per_block(T&& kernel, int num_blocks, int block_size) -> std::size_t {
std::size_t smem_size;
RuntimeDeviceCheck(cudaOccupancyAvailableDynamicSMemPerBlock(&smem_size, kernel, num_blocks, block_size));
return smem_size;
}
} // namespace host::runtime
and deduplicate with get_cc_major. Could you please help fix that? Also, the guard might not be needed after #31141

@kangwangamd

Copy link
Copy Markdown
Contributor Author

Thanks @DarkSharpness, that's a much cleaner approach — runtime.cuh already has the cuda*→hip* attribute shims (so no #ifndef USE_ROCM guard is needed there) plus get_cc_major. I'll:

  1. Move getSMVersion from utils.cuh into runtime.cuh (dropping the guard).
  2. Deduplicate against get_cc_major — add a matching get_cc_minor (and the cudaDevAttrComputeCapabilityMinor → hipDeviceAttributeComputeCapabilityMinor #define, which isn't there yet) and express getSMVersion as get_cc_major(id) * 10 + get_cc_minor(id).
  3. Note: in the JIT tree getSMVersion currently has no caller (its only reference was its own definition — the direct call was removed in Delete CUTLASS FP8 blockwise for SM90 and SM100, move SM120 to JIT and add SwapAB #30438), so no use-site change is needed here; I'll double-check after the move.

Will push shortly and re-verify the gfx950 compile.

Per @DarkSharpness: instead of guarding getSMVersion out of utils.cuh on
ROCm, move it into runtime.cuh where the cuda*->hip* attribute shims
already live, so it compiles on both CUDA and HIP with no USE_ROCM guard.
Add the cudaDevAttrComputeCapabilityMinor->hip #define + get_cc_minor,
and express getSMVersion as get_cc_major*10 + get_cc_minor to deduplicate
the compute-capability queries. No JIT-tree caller, so no use-site change.

Compile-verified on gfx950 (MI355X): the moved function compiles clean
via the HIP attribute shims (function present, not guarded out).
@kangwangamd

Copy link
Copy Markdown
Contributor Author

@DarkSharpness done — pushed the rework:

  • Removed the guarded getSMVersion from utils.cuh.
  • Moved it into runtime.cuh, added the cudaDevAttrComputeCapabilityMinor → hipDeviceAttributeComputeCapabilityMinor #define + a get_cc_minor, and defined getSMVersion(id) = get_cc_major(id) * 10 + get_cc_minor(id) to dedup the attribute queries.
  • No #ifndef USE_ROCM guard needed — the shims in runtime.cuh make it compile on both platforms. No JIT-tree caller, so no use-site change.

Compile-verified on gfx950 (MI355X, hipcc -DUSE_ROCM --offload-arch=gfx950): the moved function now compiles clean via the HIP attribute shims (present, not guarded out). PTAL 🙏

}

// Return the SM version (major * 10 + minor) for the given device
inline auto getSMVersion(int device_id) -> int {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer get_sm_version.

@DarkSharpness DarkSharpness left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM

@kangwangamd kangwangamd changed the title [AMD] Guard CUDA-only getSMVersion() for ROCm in jit_kernel utils.cuh [AMD] Move getSMVersion to runtime.cuh and fix ROCm compile (dedup with get_cc_major) Jul 14, 2026
@kangwangamd

Copy link
Copy Markdown
Contributor Author

Done — renamed to get_sm_version for snake_case consistency (6a09fb0). Thanks for the review!

yctseng0211 added a commit that referenced this pull request Jul 14, 2026
…get_cc_major)

Adopt the reviewed approach from #31141 (per @DarkSharpness): instead of
guarding CUDA-only getSMVersion with #ifndef USE_ROCM in utils.cuh, move the
compute-capability query into runtime.cuh where the cuda*->hip* attribute
shims already live, so it compiles on both CUDA and HIP with no guard. Add
the cudaDevAttrComputeCapabilityMinor->hip #define + get_cc_minor, and express
get_sm_version(id) = get_cc_major(id) * 10 + get_cc_minor(id) to dedup the
compute-capability queries. getSMVersion has no JIT-tree caller, so no
use-site change; removes the guarded getSMVersion from utils.cuh.
Co-authored-by: kangwangamd <kangwang@amd.com>
yctseng0211 added a commit that referenced this pull request Jul 14, 2026
…get_cc_major)

Adopt the reviewed approach from #31141 (per @DarkSharpness): move the CUDA-only compute-capability query out of utils.cuh into runtime.cuh where the cuda*->hip* attribute shims already live, so it compiles on both CUDA and HIP with no #ifndef USE_ROCM guard. Add the cudaDevAttrComputeCapabilityMinor->hip #define + get_cc_minor, and express get_sm_version(id) = get_cc_major(id) * 10 + get_cc_minor(id). No JIT-tree caller, so no use-site change.

Co-authored-by: kangwangamd <kangwang@amd.com>
@b8zhong b8zhong added the run-ci label Jul 14, 2026
@HaiShaw HaiShaw changed the title [AMD] Move getSMVersion to runtime.cuh and fix ROCm compile (dedup with get_cc_major) [AMD] [Not-Merge] Move getSMVersion to runtime.cuh and fix ROCm compile (dedup with get_cc_major) Jul 14, 2026
@kangwangamd

Copy link
Copy Markdown
Contributor Author

Closing this out — it was always marked [Not-Merge] (a proposal/discussion PR), and the ROCm fix it described is now on main: runtime.cuh already provides the HIP mappings for cudaDevAttrComputeCapabilityMajor/Minor (via hipDeviceAttributeComputeCapability*), so get_sm_version compiles under hipcc without the guard this PR proposed. This branch's diff is now empty (0 files) against main. Thanks @DarkSharpness for the runtime.cuh direction — that's where it landed. Closing as obsolete/superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants