Fix: raise error if device memory allocation fails (#451)

[v01dXYZ]

Previously, device memory allocation silently failed and returned the previous value stored in the result buffer. For example, a ndarray failed allocation could return a pointer to a previous allocated ndarray. Even so it would fail at memset time when zero filling it, it is wrong and confusing to debug.

Issue: #451

Brief Summary

• clear the result buffer with nullptr
• Use a DevAlloc with alloc_id := 1 to signal allocation failure

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2110a1fb8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Use an allocation size that reliably exceeds device capacity

The test hard-codes N = 0x7F_FF_FF_FF (~2 GiB), which can successfully allocate on many modern GPUs (especially on CUDA mem-pool setups), so pytest.raises(RuntimeError) becomes environment-dependent and may fail even when the code is correct. This makes the test flaky/non-deterministic across CI machines with different VRAM; the requested size should be derived to exceed available device memory in the running environment.

Useful? React with 👍 / 👎.

[hughperkins]

also can we make N more intuitive to read please?

[hughperkins]

oh I see this is old code, not part of your PR.

REquest still stands though :)

[hughperkins]

oh wait, I was looking at a single commit. So yes, it's part of your change. Let's make N intuitive to read please.

[v01dXYZ]

The reason I used this N is to make clear all bits are set (except the most significant one). I could use the equivalent (1 << 31) - 1.

[hughperkins]

what is the goal of making sure all bits are set?

[hughperkins]

did you intend to make sure all bits in 64-bits are set?

ie (1 << 63) - 1 ?

[v01dXYZ]

• setting almost all bits to 1: Just to be sure it maxes out.
• (1 << 63) - 1 is rejected by ndarray that supports only int32 indexing (same with (1 << 32) - 1 btw).

[hughperkins]

Per Claude, 31 bits is only 2GB, which sounds about right to me, since 2^32 is about 4 billion, so 2^31 is about 2 billion, which is 2GB.

Maybe you need to make multiple allocations, until you hit OOM?

[hughperkins]

can you clarify why this asserted wasnt catching the issue please.

[v01dXYZ]

It was catching the issue but I've got two options to make this error platform independent:

• put it in cuda/cuda_device.cpp as well
• or raise the error in llvm_runtime_executor.cpp instead

A chose the latter because the error message is in one place.

[hughperkins]

Opus 4.6 review:

  Two commits on this branch, touching 5 files. Here's my read:
  What it does: Replaces crash-on-failure behavior with proper error propagation when GPU memory allocation fails. Both the CUDA and AMDGPU backends now return a sentinel
  DeviceAllocation with alloc_id = -1 instead of either asserting (QD_ASSERT in AMDGPU, which would abort) or silently continuing with a null pointer (CUDA, which would crash at
   memset). The LLVM runtime executor then catches that sentinel and raises a meaningful QD_ERROR_IF.
  Looks good:
  • The overall intent is solid — turning aborts/segfaults into a catchable RuntimeError is a clear win.
  • The AMDGPU change is especially nice since it replaces a hard QD_ASSERT that would kill the process.
  • The CUDA path previously had a subtle bug: a null info.ptr would skip memset but continue as if the allocation succeeded. Now it fails explicitly.
  • The test is clean and directly exercises the intended behavior.
  • The second commit fixing qd.gpu in the test decorator makes sense — without it the test would only run on CPU where this path isn't hit.

  One concern — `runtime.cpp`:

   quadrants/runtime/llvm/runtime_module/runtime.cpp lines 952-958

                                        std::size_t size,
                                        std::size_t alignment,
                                        uint64 *result) {
  +  *result = (uint64) nullptr;
     *result = quadrants_union_cast_with_different_sizes<uint64>(
         runtime->allocate_aligned(runtime->runtime_memory_chunk, size,
                                   alignment));

  The *result = (uint64) nullptr; line is immediately overwritten by the next assignment. It's a no-op — the only scenario it would matter is if allocate_aligned threw an
  exception, but then *result wouldn't be read anyway since the stack would unwind. This line should probably be removed, or if the intent is defensive initialization, it needs
  a different approach (e.g., checking the return value after the call).
  Minor design note: Using alloc_id = -1 as a sentinel works but is a bit fragile convention-wise. If this codebase ever grows, a std::optional<DeviceAllocation> or a
  result/error type would be more self-documenting. Not a blocker for this PR though.
  Overall: good change, just that one dead line in runtime.cpp that should be cleaned up or rethought.

[hughperkins]

please check Opus's comment for this line

[v01dXYZ]

Here is a comment I put in there but removed as it was verbose:

// [ON DEVICE] At allocation failure, the runtime error flag is raised and the allocation kernel
// is aborted. If no clearing, the result would be the previous stored value (which could be for
// example the result of a previous allocation for a ndarray).
*result = nullptr;

[v01dXYZ]

On amdgpu, assert is implemented using s_endpgrm which is equivalent to return;, simply stopping the kernel as a normal exit. I don't know if the nvidia path makes the kernel sends a signal to the host process, but either way, it means *result is left untouched.

[v01dXYZ]

That's not a dumb question maybe it could be useful to mark the assert functions as [noreturn].

[hughperkins]

Please create a unit test that fails if we remove the *result = nullptr; line, to demonstrate the issue.

[hughperkins]

Asked Opus about this:

thoughts on '*result = nullptr in runtime.cpp needing a test'?

I think writing a test for that line is impractical, and the line itself is redundant. Here's why:

The *result = nullptr in runtime.cpp:916 runs on-device (GPU kernel). It's immediately overwritten by the allocate_aligned call on line 917-918. The only scenario it would
matter is if allocate_aligned triggers a device-side assert that halts the kernel before writing to *result, leaving it with a stale value.

But both callers already zero the result buffer from the host side before invoking the kernel:

• CudaDevice::allocate_llvm_runtime_memory_jit (line 87-88): memcpy_host_to_device(params.result_buffer, &zero, ...)
• AmdgpuDevice::allocate_llvm_runtime_memory_jit (line 97-98): same thing

So the on-device *result = nullptr is fully redundant with the host-side zeroing. If the kernel aborts before writing a result, the host reads back the zero it already wrote.

As for testing: you'd need to trigger a device-side assert inside allocate_aligned (pool exhaustion), then verify the host reads back null rather than a stale pointer. That's
extremely architecture-specific behavior (depends on how GPU asserts interact with memory writes) and would be very fragile.

My recommendation: the on-device *result = nullptr line could be removed since the host-side zeroing already covers the case. But it's also harmless as belt-and-suspenders.
Either way, a test for it would be testing GPU assert memory semantics, not application logic -- not worth the effort.

[hughperkins]

I will just leave it for now :)

[hughperkins]

I asked Opus: maybe it's worth using an enum or something instead of hard-coding -1?

Opus reply:

  Yeah, agreed. alloc_id is a uint64_t, so -1 silently wraps to UINT64_MAX — that works but is a magic number scattered across three files. A named constant would be much
  clearer.
  The most natural fit given the existing code style would be a constexpr next to the DeviceAllocationId typedef, plus a small helper on the struct. Something like:

  using DeviceAllocationId = uint64_t;
  constexpr DeviceAllocationId kDeviceAllocationFailed = static_cast<DeviceAllocationId>(-1);

  Then DeviceAllocation could get a convenience method:

  bool is_valid() const { return alloc_id != kDeviceAllocationFailed; }

  That turns the three usage sites into:

  // in cuda_device.cpp / amdgpu_device.cpp
  fail_alloc.alloc_id = kDeviceAllocationFailed;
  // in llvm_runtime_executor.cpp
  QD_ERROR_IF(!devalloc.is_valid(), ...);

  Reads immediately — no need to remember what -1 means. And if the sentinel value ever needs to change, it's in one place.

[hughperkins]

I asked Opus: what would be involved in your other suggestion std::optional or a
result/error type ?

Opus reply:

  Good question. Let me map out what each approach would actually touch.
  The call chain is:

  CudaDevice::allocate_memory_runtime()        ──┐
  AmdgpuDevice::allocate_memory_runtime()       ──┼──► LlvmRuntimeExecutor::allocate_memory_on_device()
  CpuDevice::allocate_memory_runtime()          ──┘         │
       ▲ (virtual, declared in LlvmDevice)                  ▼
                                                 kernel_launcher.cpp (cuda + amdgpu)
                                                 program.h / llvm_program.h

  So you'd be changing a virtual method signature across a 4-class hierarchy plus all callers. Concretely, 8 files:

  ┌────────────────────────────────────────┬──────────────────────────┐
  │ File                                   │ Change                   │
  ├────────────────────────────────────────┼──────────────────────────┤
  │ rhi/llvm/llvm_device.h                 │ Base virtual declaration │
  │ rhi/cuda/cuda_device.h                 │ Override declaration     │
  │ rhi/cuda/cuda_device.cpp               │ Override definition      │
  │ rhi/amdgpu/amdgpu_device.h             │ Override declaration     │
  │ rhi/amdgpu/amdgpu_device.cpp           │ Override definition      │
  │ rhi/cpu/cpu_device.h                   │ Override declaration     │
  │ rhi/cpu/cpu_device.cpp                 │ Override definition      │
  │ runtime/llvm/llvm_runtime_executor.cpp │ Caller                   │
  └────────────────────────────────────────┴──────────────────────────┘

  Interestingly, the codebase already has a convention for this. The sibling method allocate_memory() already uses RhiResult with an output parameter:

   quadrants/rhi/public_device.h lines 616-617

    virtual RhiResult allocate_memory(const AllocParams &params,
                                      DeviceAllocation *out_devalloc) = 0;

  And RhiResult already has an out_of_memory variant:

   quadrants/rhi/public_device.h lines 41-47

  enum class RhiResult {
    success = 0,
    error = -1,
    invalid_usage = -2,
    // ...
    out_of_memory = -4,
  };

  In fact, CpuDevice::allocate_memory_runtime already delegates to allocate_memory() internally and gets an RhiResult back — it just throws it away:

   quadrants/rhi/cpu/cpu_device.cpp lines 44-51

  DeviceAllocation CpuDevice::allocate_memory_runtime(
      const LlvmRuntimeAllocParams &params) {
    DeviceAllocation alloc;
    RhiResult res = allocate_memory(params, &alloc);
    RHI_ASSERT(res == RhiResult::success &&
               "Failed to allocate memory for runtime");
    return alloc;
  }

  So here's the comparison:

  ────────────────────────────────────────

  Option 1: `std::optional<DeviceAllocation>` — minimal, but you lose the "why":

  // llvm_device.h
  virtual std::optional<DeviceAllocation> allocate_memory_runtime(...);
  // cuda_device.cpp
  if (!info.ptr) return std::nullopt;
  // llvm_runtime_executor.cpp
  auto devalloc = llvm_device()->allocate_memory_runtime(...);
  QD_ERROR_IF(!devalloc.has_value(), "Failed to allocate...");

  Pro: Clean, modern C++, no sentinel values. Con: Tells you it failed, but not why (OOM vs invalid params vs something else).

  ────────────────────────────────────────

  Option 2: Match the existing `RhiResult` + out-param convention — more consistent with the codebase:

  // llvm_device.h
  virtual RhiResult allocate_memory_runtime(
      const LlvmRuntimeAllocParams &params, DeviceAllocation *out_devalloc);
  // cuda_device.cpp
  if (!info.ptr) return RhiResult::out_of_memory;
  // llvm_runtime_executor.cpp
  DeviceAllocation devalloc;
  RhiResult res = llvm_device()->allocate_memory_runtime(..., &devalloc);
  QD_ERROR_IF(res != RhiResult::success, "Failed to allocate...");

  Pro: Consistent with allocate_memory(), carries error reason, CpuDevice would simplify (it already gets an RhiResult internally). Con: Slightly more boilerplate,
  output-parameter style.

  ────────────────────────────────────────

  My take: Option 2 (the RhiResult pattern) is probably the right call here since the codebase already uses it for the sibling method. But it's a noticeably bigger change than
  slapping a constexpr on the sentinel — 8 files and a virtual signature change vs. 4 lines in a header.
  For this PR specifically, the constexpr kDeviceAllocationFailed approach gets you 80% of the benefit with 20% of the churn. The RhiResult refactor could be a clean follow-up
  PR.

[hughperkins]

I'm fine with the current approach. Mostly because it avoids my having to read a new much-bigger PR 😅 Lets apply the suggestions in opus's first reponse though please;

using DeviceAllocationId = uint64_t;
constexpr DeviceAllocationId kDeviceAllocationFailed = static_cast(-1);

Then DeviceAllocation could get a convenience method:

bool is_valid() const { return alloc_id != kDeviceAllocationFailed; }

[v01dXYZ]

I could add to DeviceAlloc structure a constexpr FAILED_ALLOC_ID = -1. Is that fin? Do you want to have a method to check if DevAlloc is valid?

[hughperkins]

can you outline:

• how is this different from the proposal above?
• what you see as the good and bad points of each approach?

[hughperkins]

@claude review

[hughperkins]

@claude review

[hughperkins]

@claude review

[hughperkins]

@claude review

[hughperkins]

Thanks!