Skip to content

Add CommandBuffer abstraction with backend-specific downcasting#1033

Open
MarijnS95 wants to merge 1 commit intollvm:mainfrom
Traverse-Research:commandbuffer-abstraction
Open

Add CommandBuffer abstraction with backend-specific downcasting#1033
MarijnS95 wants to merge 1 commit intollvm:mainfrom
Traverse-Research:commandbuffer-abstraction

Conversation

@MarijnS95
Copy link
Copy Markdown
Contributor

@MarijnS95 MarijnS95 commented Mar 30, 2026

Command buffer creation and management was previously spread across each backend's executeProgram() with no shared interface, making it impossible to manage command buffers from backend-agnostic code. This introduces a CommandBuffer base class on Device so that higher-level code can create and pass around command buffers without knowing the backend. Per-object allocator/pool ownership also prepares for future async execution where multiple command buffers may be in-flight with independent lifetimes.

  • DX: DXCommandBuffer owns Allocator, CmdList, Fence, Event
  • VK: VKCommandBuffer owns CmdPool, CmdBuffer; each submission creates a new CommandBuffer for independent lifetime management
  • MTL: MTLCommandBuffer wraps MTL::CommandBuffer

Device::createCommandBuffer() returns Expected<unique_ptr<CommandBuffer>> with a default "not supported" implementation.

Test plan

  • Verify DX backend compiles and passes existing tests
  • Verify VK backend compiles and passes existing tests
  • Verify MTL backend compiles and passes existing tests

🤖 Generated with Claude Code

@MarijnS95 MarijnS95 force-pushed the commandbuffer-abstraction branch from 378ec7a to 97e6af6 Compare March 30, 2026 13:27
if (!CBOrErr)
return CBOrErr.takeError();
auto CBOwner = std::move(*CBOrErr);
State.CB = &CBOwner->as<DXCommandBuffer>();
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.

Seems like it would be better if the Invocation state owned the command buffer rather than a temporary object in the function here. If we really should have two copies of it we should probably make it a shared_ptr rather than a unique_ptr.

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.

I don't think using a shared_ptr for a command buffer is a good idea. The usage of shared_ptr in the rendering backend API is there for resources that can be accessed across multiple threads. However, command buffers should be access from one thread at a time.

I am open for either storing it as a unique_ptr in the InvocationState or just passing it as an argument each time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good points, the InvocationState also lives on the stack here and owns all members, except for the backend-specific-typed command buffer which is merely a pointer reference to another stack member (which becomes dangling when CBOwner is dropped from the stack before InvocationState).

I also vouch for keeping it uniquely owned, that way command buffers can only be passed by reference and not accidentally held and modified in multiple places (trying very hard to forget Rust's natural ownership semantics and page in C++'s nonsensical ones 🙃).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only downside of that is needing the backend-specific pointer easily accessible. Either the field is stored twice, or this backend-specific executeProgram() call uses the specific XxxCommandBuffer::create() call to immediately have the right type until every internal field access is replaced with a generic abstraction.

@MarijnS95 MarijnS95 force-pushed the commandbuffer-abstraction branch 2 times, most recently from 8803d26 to dadf52d Compare April 1, 2026 09:33
Comment on lines +321 to +326
ComPtr<ID3D12Fence> Fence;
#ifdef _WIN32
HANDLE Event = nullptr;
#else // WSL
int Event = -1;
#endif
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.

Can we get rid of this by using the abstract Fence type introduced in #1007?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah will do once that PR is merged. Although at this point I'm weary of the fact that the fence is stored in the CommandBuffer object in the first place, despite it being single-use anyway.

@MarijnS95 MarijnS95 force-pushed the commandbuffer-abstraction branch from dadf52d to 4efc0b0 Compare April 1, 2026 13:59
@MarijnS95 MarijnS95 force-pushed the commandbuffer-abstraction branch 2 times, most recently from 5a2952f to b3ddfc2 Compare April 2, 2026 14:15
Command buffer creation and management was previously spread across
each backend's executeProgram() with no shared interface, making it
impossible to manage command buffers from backend-agnostic code. This
introduces a CommandBuffer base class on Device so that higher-level
code can create and pass around command buffers without knowing the
backend. Per-object allocator/pool ownership also prepares for future
async execution where multiple command buffers may be in-flight with
independent lifetimes.

- DX: DXCommandBuffer owns Allocator, CmdList, Fence, Event
- VK: VKCommandBuffer owns CmdPool, CmdBuffer; each submission
  creates a new CommandBuffer for independent lifetime management
- MTL: MTLCommandBuffer wraps MTL::CommandBuffer

Device::createCommandBuffer() returns Expected<unique_ptr<CommandBuffer>>
with a default "not supported" implementation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@MarijnS95 MarijnS95 force-pushed the commandbuffer-abstraction branch from b3ddfc2 to 095fc23 Compare April 7, 2026 10:10
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.

4 participants