Skip to content

[UR][HIP] Opt into the larger per-block shared-memory pool - #23419

Open
zjin-lcf wants to merge 5 commits into
intel:syclfrom
zjin-lcf:hip-local-mem-optin
Open

zjin-lcf wants to merge 5 commits into
intel:syclfrom
zjin-lcf:hip-local-mem-optin

Conversation

@zjin-lcf

@zjin-lcf zjin-lcf commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The HIP adapter reported hipDeviceAttributeMaxSharedMemoryPerBlock as UR_DEVICE_INFO_LOCAL_MEM_SIZE and called hipFuncSetAttribute(hipFuncAttributeMaxDynamicSharedMemorySize) only when UR_HIP_MAX_LOCAL_MEM_SIZE / SYCL_PI_HIP_MAX_LOCAL_MEM_SIZE was set. That matches neither the CUDA adapter nor HIP on NVIDIA.

On NVIDIA, HIP forwards these calls to CUDA. The default per-block limit stays 48 KiB, and the opt-in ceiling is larger (measured: T4 64 KiB, RTX A2000 99 KiB, H100 PCIe 227 KiB). A launch above 48 KiB has to opt in with cudaFuncSetAttribute / hipFuncSetAttribute. The CUDA adapter already does this; the HIP adapter did not, so a SYCL program on the HIP backend could not use that pool.

This change:

  • Queries hipDeviceAttributeSharedMemPerBlockOptin and reports the larger of that value and the non-opt-in size as local_mem_size.
  • On enqueue, sets hipFuncAttributeMaxDynamicSharedMemorySize when the kernel's dynamic local size exceeds the non-opt-in limit.
  • Keeps UR_HIP_MAX_LOCAL_MEM_SIZE as a cap on that ceiling.

A failed query or a zero opt-in result keeps the non-opt-in size. ROCm 5.7 rejects the attribute on AMD, and ROCm 6.2 returns 0. ROCm 6.4+ reports the opt-in value, but it is the same 64 KiB LDS size as the non-opt-in limit for every ISA in the ROCclr table (gfx906, gfx90a, gfx942, and the RDNA targets). On those AMD devices this does not grow the pool; it also does not shrink local_mem_size to 0.

Fixes #23365.

Test plan

  • MI210 (gfx90a, ROCm 7.1.1), both devices, with the patched libur_adapter_hip.so: local_mem_size is 65536; 1 KiB, 48 KiB, and 64 KiB local_accessor launches read back the first and last byte; 64 KiB + 1 KiB is rejected before launch.
  • UR_HIP_MAX_LOCAL_MEM_SIZE=32768 lowers both the query and the launch limit. A cap above the device size is clamped to 65536.
  • Native HIP attribute query on MI210 and MI50/MI60: opt-in equals the non-opt-in 64 KiB under ROCm 6.4/7.1, is 0 under ROCm 6.2, and the query is rejected under ROCm 5.7. A 64 KiB dynamic launch succeeds without hipFuncSetAttribute; 65 KiB fails with or without it.
  • gfx942 (MI300A, ROCm 7.2.4), patched adapter, devices 0 and 3: opt-in equals the non-opt-in 65536, so sycl/test-e2e/Adapters/hip-local-mem-optin.cpp skips the default + 1 launch. local_mem_size is 65536; a 64 KiB launch reads back the first and last byte; 64 KiB + 1 is rejected. UR_HIP_MAX_LOCAL_MEM_SIZE=32768 lowers both the query and the launch limit.
  • HIP program on an H100 PCIe (HIP_PLATFORM=nvidia, ROCm 7.1.1 HIP, CUDA 12.8). hipDeviceAttributeMaxSharedMemoryPerBlock is 49152 and hipDeviceAttributeSharedMemPerBlockOptin is 232448 (227 KiB). A dynamic launch of 48 KiB succeeds with no hipFuncSetAttribute. 49153 bytes fails with hipErrorInvalidValue unless hipFuncAttributeMaxDynamicSharedMemorySize is set to the launch size (or once, to 232448). The opted-in launch then succeeds through 232448 bytes and reads back the first and last byte; 232449 is rejected. This is a native HIP program, not a SYCL HIP-adapter launch.
  • HIP program on MI210 (gfx90a, ROCm 7.1.1). Both attributes are 65536. A 64 KiB dynamic launch succeeds with no hipFuncSetAttribute. 65537 fails with or without it, and setting the attribute to 65537 is itself hipErrorInvalidValue.

Made with Cursor

The HIP adapter reported only the non-opt-in shared-memory size and set
hipFuncAttributeMaxDynamicSharedMemorySize only when
UR_HIP_MAX_LOCAL_MEM_SIZE was set. On NVIDIA that hides the opt-in pool
above 48 KiB. Query the opt-in ceiling and set the attribute when a
launch exceeds the default limit, matching the CUDA adapter. A missing
or zero opt-in result keeps the non-opt-in size.

Co-authored-by: Cursor <cursoragent@cursor.com>
@zjin-lcf
zjin-lcf requested a review from a team as a code owner October 8, 2026 21:00
@zjin-lcf
zjin-lcf requested a review from kekaczma October 8, 2026 21:00
@bratpiorka
bratpiorka requested a balanced review from Copilot October 14, 2026 10:48

Copilot AI 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.

🟡 Changes recommended

The central HIP-on-NVIDIA opt-in launch path lacks automated coverage.

1 open finding
What changed in this PR

Aligns the HIP adapter with CUDA’s opt-in shared-memory behavior.

Changes:

  • Reports the larger opt-in local-memory ceiling with fallback.
  • Opts kernels into larger dynamic shared memory when needed.
  • Preserves environment-variable caps.
File Description
hip/​enqueue.cpp Enables per-kernel opt-in shared memory.
hip/​device.hpp Queries and stores default and opt-in limits.
hip/​device.cpp Documents the reported local-memory ceiling.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread unified-runtime/source/adapters/hip/enqueue.cpp Outdated
The opt-in attribute path is only reachable when the per-block ceiling
exceeds the non-opt-in limit. Skip that launch when the two are equal,
and still check the reported size, a launch at that size, and rejection
of one extra byte, including a UR_HIP_MAX_LOCAL_MEM_SIZE cap.
@zjin-lcf
zjin-lcf requested a review from a team as a code owner October 14, 2026 14:26
@ldorau
ldorau requested a balanced review from Copilot October 15, 2026 09:37

Copilot AI 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.

Comment thread sycl/test-e2e/Adapters/hip-local-mem-optin.cpp Outdated
Comment thread sycl/test-e2e/Adapters/hip-local-mem-optin.cpp
Comment thread unified-runtime/source/adapters/hip/enqueue.cpp Outdated
Comment thread unified-runtime/source/adapters/hip/device.hpp Outdated
Comment thread unified-runtime/source/adapters/hip/enqueue.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Comment thread sycl/test-e2e/Adapters/hip-local-mem-optin.cpp Outdated
Comment thread unified-runtime/source/adapters/hip/enqueue.cpp Outdated
Comment thread unified-runtime/source/adapters/hip/enqueue.cpp Outdated
Comment thread unified-runtime/source/adapters/hip/enqueue.cpp Outdated

@ldorau ldorau 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.

Please fix failing CI builds

Co-authored-by: Cursor <cursoragent@cursor.com>
@zjin-lcf

Copy link
Copy Markdown
Contributor Author

Addressed the latest review in 475f087:

  • select the HIP backend explicitly and require hip_dev_kit in the E2E test;
  • validate static + dynamic shared memory before opting in;
  • configure one stable, capacity-bounded dynamic limit per module function, shared across UR kernel handles;
  • use cuFuncSetAttribute for NVIDIA module functions and retain hipFuncSetAttribute for AMD;
  • cache the static-memory query/configuration and clarify the HIP last-error comment.

Hardware validation on gilgamesh:

  • MI210 (gfx90a, ROCm 7.1.1): the updated AMD HIP adapter builds successfully. The E2E test passes uncapped (65536-byte launch succeeds, 65537 is rejected) and with UR_HIP_MAX_LOCAL_MEM_SIZE=32768 (32768 succeeds, 32769 is rejected).
  • H100 PCIe (CUDA 12.8, ROCm 7.1.1 HIP headers): a module-function probe using hipModuleGetFunction + cuFuncSetAttribute reports default 49152 and opt-in 232448. A 232448-byte dynamic launch succeeds. A second module kernel reports 16384 bytes of static shared memory; setting the stable dynamic maximum to 232448 - 16384 makes a 40 KiB dynamic launch succeed, covering the static+dynamic threshold case.

The changed NVIDIA program.cpp compiles with __HIP_PLATFORM_NVIDIA__. A complete local HIP-on-NVIDIA adapter link was not available because this checkout has unrelated existing build failures in image/command-buffer code against the installed HIP-on-NVIDIA headers; the H100 validation therefore exercised the exact driver/module API path directly rather than through a linked UR adapter.

Co-authored-by: Cursor <cursoragent@cursor.com>
@zjin-lcf

Copy link
Copy Markdown
Contributor Author

The actionable Windows CI failure was the new HIP test being selected by the build-only Windows configuration: %{build} targeted SPIR-V while %hip_options added --rocm-path, which became an unused-argument error under -Werror. Commit cc63e0a marks this HIP hardware test unsupported on Windows. The separate BMG job ran all 1,359 selected tests successfully; it failed during cleanup because an unrelated bindless_images/aligned_alloc_device_2d process remained hung and kept the build directory busy.

The Windows build compiles this test for spir64, where --rocm-path from
%hip_options is unused and fails under -Werror.

Co-authored-by: Cursor <cursoragent@cursor.com>

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HIP adapter does not opt into MaxSharedMemoryPerBlockOptin

3 participants