Introduce a new Buffer type with DX,VK,MTL implementations. by manon-traverse · Pull Request #976 · llvm/offload-test-suite

manon-traverse · 2026-03-16T13:49:23Z

A first small step towards creating a Render Backend API layer on top of DX12, Vulkan, and Metal.

This introduces a new Buffer type with an implementation for each API as well as a function on the Device class to create one.

The change is smaller than it looks due to a naming conflict that needed to be resolved. The type previously called Buffer has been renamed to CPUBuffer.

lib/API/VK/Device.cpp

scuff3drook · 2026-03-16T19:43:16Z

include/API/Device.h

+
+struct BufferCreateDesc {
+  MemoryLocation Location;
+  BufferUsage Usage;


I don't think I see this Usage flag used anywhere. Is the idea that buffer usage describes the buffer as CBV/UAV/SRV (in DX12 parlance)?

The Usage tag will later be used to specify the type of buffer (Storage, AccelerationStructure, Constant, etc). I included it here so that the field should be filled in from the start. Perhaps we should apply a constructor on this type to enforce filling in all fields?

FWIW, I think I'd hold off on adding fields / enums until we can include code demonstrating what they're for, so I'd recommend just removing the Usage field & enum until we have a need for it.

Let's go with that, I have removed the field.

scuff3drook · 2026-03-16T19:46:03Z

include/API/Device.h

 };

+enum class MemoryLocation {
+  GpuOnly,


This may be confusing if later on you want to add CPU-visible memory which is still GPU local (ReBAR).

I wonder if the memory locations should just be GpuLocal or HostLocal and then the usage of Readback, Upload, ShaderMutable, or ShaderVisible then dictates the heap/memory-type in conjunction with "MemoryLocation"

In our closed-source render framework, we use CpuToGpu to express that the resource has to be HOST visible and won't be read from. Whether that resource is actually in GPU Local ReBAR, some form of shared memory or on HOST. I am basically using that as a starting point as it has proven to have worked so far, but I am open for suggestion and changes.

That's why the terms GpuLocal and HostLocal may not fit as well, since it's a detail that the rendering backend should deal with and not necessarily the cross-platform rendering code.

Sure, but the enum name here is MemoryLocation after all. I think hiding that as an implementation detail totally makes sense, but it may make sense for the type name to then indicate as such. Personally, if the goal is to expose strictly semantics to the user of this API, and the specific memory location is an implementation detail, I would probably opt to just expose the BufferUsage flag alone.

Fortunately, we can change code in the future if we decide that the abstraction we've picked doesn't work out well. At this point I think that categorizing memory as GpuOnly, CpuToGpu and GpuToCpu is a reasonable categorization that works well across all the APIs. I do agree that the name MemoryLocation could be tightened up - MemoryType perhaps? I'm biased as I'm more familiar with D3D12_HEAP_TYPE that has 1:1 mappings with these values :)

MemoryType doesn't match with Vulkan memory types, though. The name of this enum is not as important to me as long as I can quickly understand if it's CpuToGpu, GpuOnly, or GpuToCpu.
The reason for MemoryLocation was just purely based on it being called that in our in-house rendering framework.

Ah, trying to pick good names is hard, trying to pick good names that don't conflict with names used by 3 graphics APIs is even harder :)

I don't think we should block this PR going in on getting the right name. I agree that the abstraction here is a good one, and I think we'll have to accept that the name picked here is somewhat arbitrary and the least-obviously-bad one.

lib/API/DX/Device.cpp

scuff3drook · 2026-03-16T19:47:21Z

lib/API/DX/Device.cpp

+    }
+
+    const D3D12_RESOURCE_FLAGS Flags =
+        D3D12_RESOURCE_FLAG_ALLOW_UNORDERED_ACCESS;


These flags aren't yet used, but I'm assuming UAV access should be conditional depending on usage

On Buffers UAV access is kind-of the default. There is no special swizzling or compression like on textures. There may be some exceptions when it comes to acceleration structures that according to the spec shouldn't be UAV. However, I would like to solve that problem once we reach that point.

Yea that's definitely true (re: difference between buffers and textures), although the semantic difference for buffers is often still meaningful in most engines -- you wouldn't expect to create a UAV descriptor for a read-only buffer for example (unless you live in a BDA-only world)

From my experience, having worked in various game engines, I have never seen an advantage to marking buffers as read/SRV-only at buffer creation time. Just binding it as read or write has been sufficient as that also guides barrier placement.

I would also like to point out what this flag is for. This flag is just purely there to tell the driver what kind of optimizations (swizzling/compression) it can apply on a resource. That's why resources with no flags passed disallow a lot of use cases. However, in the case of buffers (which use the same resource creation API) there are no optimziations to apply. So there is no point to not passing this flag in the rendering backend.

llvm-beanz

A couple comments. As a meta-comment, we do use clang-tidy to enforce consistent style and coding standards. That is run with -Werror in our bots, so some of the CI failures you're seeing as build failures are Clang-Tidy failures.

Our ReadMe documents setting up Clang-Tidy once you have it installed: https://github.com/llvm/offload-test-suite?tab=readme-ov-file#enabling-clang-tidy

llvm-beanz · 2026-03-17T16:05:46Z

lib/API/DX/Device.cpp

+class DXBuffer : public offloadtest::Buffer {
+public:
+  ComPtr<ID3D12Resource> Buffer;
+  std::string Name;


Should we put the Name and SizeInBytes in the base class? Seems like all the sub-classes have those fields.

I am not sure! I am a big fan of using Pure Virtual Classes and only doing functionality inheritance without any data inheritance, much like traits in Rust. What is the LLVM approach to this?

lib/API/VK/Device.cpp

manon-traverse · 2026-03-17T16:31:08Z

A couple comments. As a meta-comment, we do use clang-tidy to enforce consistent style and coding standards. That is run with -Werror in our bots, so some of the CI failures you're seeing as build failures are Clang-Tidy failures.

Our ReadMe documents setting up Clang-Tidy once you have it installed: https://github.com/llvm/offload-test-suite?tab=readme-ov-file#enabling-clang-tidy

Yes I saw something along the lines of that. I'll check it out! <3

I have some local changes to the README with steps and pointers that I found difficult to figure out to improve the onboarding in the future.

damyanp

LGTM, just a couple of observations.

damyanp · 2026-03-26T17:46:20Z

lib/API/DX/Device.cpp

+  ComPtr<ID3D12Resource> Buffer;
+  std::string Name;
+  BufferCreateDesc Desc;
+  size_t SizeInBytes;


Did we turn off the clang-tidy rule about public data members?

damyanp · 2026-03-26T17:51:26Z

lib/API/DX/Device.cpp

  Queue &getGraphicsQueue() override { return GraphicsQueue; }

+  llvm::Expected<std::shared_ptr<offloadtest::Buffer>>
+  createBuffer(llvm::StringRef Name, BufferCreateDesc &Desc,


I don't think we actually have any code calling createBuffer() with this PR, which does make it hard for me to guess on usage here. But...it does slightly bother me that I think we've got a potentially unnecessary string copy going on here. Is createBuffer took a std::string value, then that'd let the caller decide whether or not they want to move a string to it or create a copy of another string to give to it. The current implementation only supports copying the string.

manon-traverse commented Mar 16, 2026

View reviewed changes

lib/API/VK/Device.cpp Outdated Show resolved Hide resolved

scuff3drook reviewed Mar 16, 2026

View reviewed changes

llvm-beanz reviewed Mar 17, 2026

View reviewed changes

manon-traverse force-pushed the render-backend-api-buffer branch 4 times, most recently from 1bd0067 to 7f99fc7 Compare March 24, 2026 15:17

manon-traverse mentioned this pull request Mar 24, 2026

Implement an abstract Fence type to expose waiting for command buffer completion. #1007

Draft

EmilioLaiso mentioned this pull request Mar 26, 2026

Introduce a new Texture type with DX,VK,MTL implementations. #1020

Open

manon-traverse added 7 commits March 26, 2026 16:34

Introduce a new Buffer type with DX,VK,MTL implementations.

10dc775

Add heap type selection to DX12 and clean up.

7d9a3c3

Remove redundant comments.

7f5d5e2

Use the selected HeapType.

9354c6e

Remove unnecessary default labels.

20b379b

Fix up after rebase.

d7c1216

Fix typo causing metal to fail compiling.

b67a337

manon-traverse force-pushed the render-backend-api-buffer branch from 7f99fc7 to b67a337 Compare March 26, 2026 15:38

manon-traverse requested a review from llvm-beanz March 26, 2026 15:55

Remove BufferUsage for now.

e7fd424

damyanp approved these changes Mar 26, 2026

View reviewed changes

Conversation

manon-traverse commented Mar 16, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scuff3drook Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manon-traverse commented Mar 17, 2026

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

scuff3drook Mar 18, 2026 •

edited

Loading