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.

The advantage just in a dx12 sense is that you can validate against incorrect usage if anyone tries to grab a UAV descriptor basically. Not a hill I'd ever die on. (I've worked on console games for over a decade and 5 different game engines now so I'm familiar with delta color compression and the other concepts you are mentioning FWIW). Some engines map descriptors/addresses persistently and expose different accessors depending on declared usage, so it's a question of style and user preference I suppose.

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?

I don't think LLVM has a strict opinion here, both patterns are present depending on the context. I think the functionality inheritance here is reasonable.

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?

clang-tidy doesn't seem to mind. Personally, I don't see the advantage of private data members and using getters and setters for everything. It adds a lot of boilerplate to solve issues that are these days solved by tools like clang-tidy (for example: if (MyObject->Field = 3). On top of that, if you write MyObject->GetField() everywhere, assuming that that is a cheap (should-be-free) operation, you can get in the situation where GetField() is refactored to be an expensive operation, you suddenly have introduced a lot of performance overhead. By just accessing the fields directly you are force to refactor the call site as well when changes are made and can catch these types of issues more easily.

If, however, using private fields with getters and setters is the LLVM way, we will adapt to that.

I tend to agree with you here - I only mentioned it because I've had to fight with clang-tidy over this in the past :)

Our clang-tidy rules are setup to match LLVM's coding standards, and LLVM allows public data members in classes.

offload-test-suite/.clang-tidy

Line 1 in bc74ffb

Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-use-anonymous-namespace,-misc-include-cleaner,-misc-non-private-member-variables-in-classes,readability-identifier-naming'

It functionally 100% equivalent, so I'm happy with whatever you do here, but typically LLVM uses struct for all-public objects like this. Saves a line of code since it's the default and there's a very weak argument that it makes things clearer.

lib/API/DX/Device.cpp

damyanp · 2026-03-31T16:34:29Z

Can you fix the code formatting errors? Then I can merge this.

bogner

This generally looks reasonable, but for future reference it's always nice if we can add some usage and/or tests whenever we add API surface like this. This both helps in clarifying the motivation and makes it easier to review when you can see the usage in context.

bogner · 2026-04-01T04:17:02Z

lib/API/DX/Device.cpp

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


I don't think LLVM has a strict opinion here, both patterns are present depending on the context. I think the functionality inheritance here is reasonable.

bogner · 2026-04-01T04:18:45Z

lib/API/DX/Device.cpp

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


It functionally 100% equivalent, so I'm happy with whatever you do here, but typically LLVM uses struct for all-public objects like this. Saves a line of code since it's the default and there's a very weak argument that it makes things clearer.

bogner · 2026-04-01T04:22:52Z

include/API/Device.h


  virtual Queue &getGraphicsQueue() = 0;

+  virtual llvm::Expected<std::shared_ptr<Buffer>>


I'd prefer to avoid shared_ptr unless we really need it. Since this code just introduces the APIs and doesn't use them yet it's hard for me to tell if we do, but generally I think shared_ptr can be used in ways that obscure ownership and we should use it sparingly.

bogner · 2026-04-01T04:28:48Z

bah - github is failing to refresh these pages lately. I didn't realize my review was post commit.

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

Open

EmilioLaiso mentioned this pull request Mar 26, 2026

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

Open

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

damyanp approved these changes Mar 26, 2026

View reviewed changes

manon-traverse added 9 commits March 30, 2026 18:04

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

882a0be

Add heap type selection to DX12 and clean up.

25ca67e

Remove redundant comments.

87c1660

Use the selected HeapType.

a58ac8f

Remove unnecessary default labels.

424ceb5

Fix up after rebase.

0f418a9

Fix typo causing metal to fail compiling.

067dbab

Remove BufferUsage for now.

53d6794

Use std::string instead of llvm::StringRef.

5d60f9f

manon-traverse force-pushed the render-backend-api-buffer branch from deed800 to 5d60f9f Compare March 30, 2026 16:05

llvm-beanz approved these changes Mar 30, 2026

View reviewed changes

Fix formatting.

b350d9a

damyanp merged commit 60b8173 into llvm:main Mar 31, 2026
9 of 12 checks passed

bogner approved these changes Apr 1, 2026

View reviewed changes


		virtual Queue &getGraphicsQueue() = 0;

		virtual llvm::Expected<std::shared_ptr<Buffer>>

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!

scuff3drook Mar 27, 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!

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!

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!

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!

damyanp commented Mar 31, 2026

Uh oh!

Uh oh!

bogner 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!

Choose a reason for hiding this comment

Uh oh!

bogner commented Apr 1, 2026

Uh oh!

Reviewers

scuff3drook Mar 18, 2026 •

edited

Loading

scuff3drook Mar 27, 2026 •

edited

Loading