[Metal] Explicit root signature layout for Metal backend by kcloudy0717 · Pull Request #1061 · llvm/offload-test-suite

kcloudy0717 · 2026-04-06T18:25:44Z

This PR changes the automatic linear layout binding model to the more flexible explicit root signature binding model.

Notable changes:

We no longer invokes -metal && -Fre to the compiler command line in LIT config. Reason for this is because explicit root signature model requires the RS to be bound to the IR compiler during IR conversion time.
Introduced MTLTopLevelArgumentBuffer (TLAB) class that replaces the legacy MTL::Buffer *ArgsBuffer in InvocationState. This class encapsulates a buffer for the backing memory of the data of all IR resources for the given root signature. Provides D3D12-like functions for binding root constants, root descriptors, and descriptor tables.
Introduced MTLDescriptorHeap class that replicates D3D12's descriptor heap concept. It serves as a storage for descriptors, binding a descriptor table to the TLAB must come from a descriptor heap just like D3D12. This bridge the gap between D3D12/Metal API differences allowing future tests such as dynamic resource indexing and unbound resource arrays easy to support.
Resource/Descriptor creation is similar to the D3D12 backend thanks to MTLTopLevelArgumentBuffer and MTLDescriptorHeap, this allows resource arrays to be supported in the Metal backend now.

kcloudy0717 · 2026-04-06T18:26:46Z

Pinging @bogner, @bob80905, @farzonl, @manon-traverse for review.

bogner · 2026-04-06T23:33:43Z

+#define IR_RUNTIME_METALCPP
+#include "MTLDescriptorHeap.h"
+#include "metal_irconverter_runtime.h"


Better to limit the IR_RUNTIME_METALCPP to the code that needs it:

#pragma push_macro("IR_RUNTIME_METALCPP") #include "metal_irconverter_runtime.h" #pragma pop_macro("IR_RUNTIME_METALCPP")

If we're going to use these APIs in more places it might even be worth wrapping it in a header that just does that so it's harder to make a mistake.

(I'd also make an argument for adding a MetalIRConverter.cpp and moving the IR_PRIVATE_IMPLEMENTATION inclusion of this header to that instead of Device, but that's quickly getting out of scope for this particular change)

bogner · 2026-04-06T23:35:22Z

+#include "llvm/Support/Error.h"
+#include <memory>
+
+struct IRDescriptorTableEntry;


It's a bit awkward that metal_irconverter_runtime.h just puts all of its definitions in the global namespace. Maybe a comment saying this forward declaration is for an object from there would be helpful.

bogner · 2026-04-06T23:45:33Z

+struct IRDescriptorTableEntry;
+
+namespace offloadtest {
+struct METAL_GPU_DESCRIPTOR_HANDLE {


Let's stick to LLVM naming conventions for these types (ie, MetalGPUDescriptorHandle)

bogner · 2026-04-06T23:55:29Z

+enum METAL_DESCRIPTOR_HEAP_TYPE {
+  METAL_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV,
+  METAL_DESCRIPTOR_HEAP_TYPE_SAMPLER,
+};


This is a good place for enum class, then we don't need the METAL_DESCRIPTOR_HEAP_TYPE on all of the values.

I'm also not entirely sure about the CBV or SRV or UAV value - is there a clearer name for this? I suspect we'll either actually want three different values here or that we can get away with something like Sampler and NotSampler

Ideally we keep the descriptor heap API as close to D3D12 as possible. The enum defined here maps to D3D12's enum:

typedef enum D3D12_DESCRIPTOR_HEAP_TYPE { D3D12_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV = 0, D3D12_DESCRIPTOR_HEAP_TYPE_SAMPLER, D3D12_DESCRIPTOR_HEAP_TYPE_RTV, D3D12_DESCRIPTOR_HEAP_TYPE_DSV, D3D12_DESCRIPTOR_HEAP_TYPE_NUM_TYPES } ;

Having three separate values for each CBV/SRV/UAV would make dynamic resource indexing via ResourceDescriptorHeap not possible.

FWIW, I think we can just change this to enum class and drop the METAL_DESCRIPTOR_HEAP_TYPE_ prefix for the values. I can't really think of a better name for CBV_SRV_UAV so I would say we just keep as it is.

bogner · 2026-04-07T00:10:41Z

+
+#include "MTLDescriptorHeap.h"
+#include "Metal/Metal.hpp"
+#include "metal_irconverter.h"


Do we need all of this for the header?

Yes, but I think I can get away with forward declarations for some of them. I'll do that.

bogner · 2026-04-07T00:14:23Z

We shouldn't be checking a dylib in to the repo. I didn't realize that metal_irconverter wasn't distributed header only like metal_irconverter_runtime when I suggested we could just add the dependency - this makes things more complicated unfortunately.

What would be the preferred approach? Maybe we can use CMake Fetch_Content to make this bit simpler?

I just realized the tests aren't being ran probably because of this dylib dependency. Do let me know if there is a better approach for this.

IIUC the metal-shaderconverter tool and this dylib are distributed together, and we already have the dependency on the tool. I guess what we really want to do is wire up some cmake logic to find where the dylib is installed and link to that one.

@llvm-beanz does this sound right to you?

Given that we already have this as a dependency and can safely assume the machines have the IR converter installed, this makes the logic of wiring up the dylib simple.

The document specifies the directory which the tool will be installed:

The installer contains the command-line tools, dynamic libraries, header files and documentation. On
installation you will find:

/usr/local/bin/ contains the metal-shaderconverter tool.

/usr/local/include/ contains the headers for the metal_irconverter library .

/usr/local/lib/ contains the metal_irconverter library for macOS.

/usr/local/lib_iOS/ contains the metal_irconverter library for iOS.

/opt/metal-shaderconverter/ contains Metal Shader Converter documentation.

I modified the CMake script to use find_library to retrieve the dylib:

set(METAL_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/third-party/metal-cpp /usr/local/include/metal_irconverter ${CMAKE_CURRENT_SOURCE_DIR}/third-party/metal_irconverter_runtime) find_library(METAL_IRCONVERTER_LIBRARY metalirconverter PATHS /usr/local/lib REQUIRED) set(METAL_LIBRARIES ${METAL_IRCONVERTER_LIBRARY})

It looks like the libmetalirconverter.dylib is not in the /usr/local/lib. Do we know where it's downloaded? Also maybe it'd be a better idea for us to have a CMake option to specify the path of the dylib, as the install location could potentially be different.

Scratch what I said, it looks like I needed to link the dylib for the api-query and offloader. Metal checks on CI is green.

manon-traverse · 2026-04-09T09:17:44Z

+      // Maybe there is a better way to detect GPUAPI?
+#ifdef __APPLE__
+      APIToUse = GPUAPI::Metal;
+      outs() << "Using Metal API\n";
+#else
      APIToUse = GPUAPI::DirectX;
      outs() << "Using DirectX API\n";
+#endif


This seems fine to me as Metal is Apple only.

manon-traverse

Nice work!
My main complaint in this code is a pattern around resource and descriptor that stems from how the DX12 backend is setup. However, this PRs brings the backends closer together so this will make fixing that pattern quite easy!

bogner

Generally looking pretty good. Mostly minor comments but I wouldn't mind one more look over before I'm ready to approve.

bogner · 2026-05-06T06:36:19Z

+//
+//
+//===----------------------------------------------------------------------===//


We don't need an empty comment block here - if we have things to say about the file this is the place but it's totally reasonable to omit it instead of leaving it blank.

(also applies later in the patch)

bogner · 2026-05-06T06:36:46Z

@@ -0,0 +1,72 @@
+#include "MTLDescriptorHeap.h"


Missing file header - we do want the license statement in every file.

bogner · 2026-05-06T06:46:42Z

+}
+
+MTLGPUDescriptorHandle &
+MTLGPUDescriptorHandle::Offset(int32_t OffsetInDescriptors) {


Two things here:

While Offset is certainly useable as a verb, it is also a noun. This function doesn't necessarily sound like it's modifying the object. Maybe something like increment or addOffset would be better?

LLVM style says member functions should start with a lower case letter.

bogner · 2026-05-06T06:49:54Z

+  if (!Buffer)
+    return;


Is silently doing nothing really the right thing to do here? Should we assert that Buffer isn't null instead? I assume calling bind on an ArgumentBuffer without a Buffer is a programmer error here.

This mostly follows D3D12's root signature behavior. In D3D12, empty root signatures are allowed, binding a empty root signature is a no-op. I went with this approach given the similarity of the root signature APIs, if this isn't desired, we can remove the check here and assert instead.

Fair enough. This seems fine like this.

bogner · 2026-05-06T06:50:42Z

+// Manages a Metal buffer that serves as the top-level argument buffer for
+// shader resource binding with the explicit root signature layout.
+class MTLTopLevelArgumentBuffer {
+  std::vector<IRResourceLocation> ResourceLocs;


Better to use llvm::SmallVector here - https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h

kcloudy0717 · 2026-05-07T08:57:08Z

@manon-traverse @bogner I fixed conflicts after rebasing to latest main. Branch should be good to go, just waiting on CI, feel free to have a last look.

…ckend

bogner · 2026-05-11T17:43:04Z

The metal failures are #1166, which has been fixed since those ran, and this is NFC anywhere other than Mac. I'm going to go ahead and merge this for you. Thanks!

A couple of warnings for unused variables slipped in. Clean them up.

Most of these really should've been XFAIL rather than UNSUPPORTED, but in any case, after #1061 they're supported. These all pass now except `Feature/ResourceArrays/array-local2.test`, which fails for the same reason the test fails on DirectX.

bogner reviewed Apr 7, 2026

View reviewed changes

manon-traverse reviewed Apr 8, 2026

View reviewed changes

Comment thread include/Support/Pipeline.h

manon-traverse reviewed Apr 9, 2026

View reviewed changes

kcloudy0717 force-pushed the kcloudy0717/metal_irconverter branch 4 times, most recently from 346ee84 to 9211702 Compare April 16, 2026 09:02

kcloudy0717 force-pushed the kcloudy0717/metal_irconverter branch from 3621a54 to da6cdff Compare April 26, 2026 15:27

kcloudy0717 requested review from bogner and manon-traverse April 27, 2026 05:17

kcloudy0717 force-pushed the kcloudy0717/metal_irconverter branch 3 times, most recently from 532e4a6 to a474316 Compare May 4, 2026 16:41

manon-traverse approved these changes May 5, 2026

View reviewed changes

bogner reviewed May 6, 2026

View reviewed changes

bogner approved these changes May 6, 2026

View reviewed changes

kcloudy0717 force-pushed the kcloudy0717/metal_irconverter branch from 4565630 to a8b1453 Compare May 7, 2026 08:54

manon-traverse approved these changes May 7, 2026

View reviewed changes

kcloudy0717 added 10 commits May 11, 2026 23:50

Add metal_irconverter includes/libs to third-party

ed72897

Proof of concept of using explicit root signature layout for Metal ba…

ea80cc7

…ckend

Removed XFAIL for unexpectedly passing tests that is expected to pass

5c75274

Apply LLVM naming conventions

5be3196

Cleanup headers and added more forward declarations

c5a6df1

Cleanup comment and removed dead code path

b668f0d

Removed metalirconverter.dylib, use the one installed on the system

e88c319

Removed XFAIL for passing tests

be01698

Removed metal_irconverter headers, used the one in usr/local/include

8fb0183

Attempt to fix dylib error on CI machines

9aeace2

kcloudy0717 added 5 commits May 11, 2026 23:50

Removed XFAIL for passing tests

d0357a2

Removed XFAIL for passing tests llvm#2

5b4b465

Fixup after rebase

d88e843

Address feedback

9858a2d

Fixup after rebase

38f1bb9

kcloudy0717 force-pushed the kcloudy0717/metal_irconverter branch from a8b1453 to 38f1bb9 Compare May 11, 2026 15:53

bogner merged commit e8844e4 into llvm:main May 11, 2026
11 of 17 checks passed

bogner added a commit that referenced this pull request May 11, 2026

Fix -Wunused warnings after #1061 (#1171)

6d97dc1

A couple of warnings for unused variables slipped in. Clean them up.

bogner mentioned this pull request May 11, 2026

[Metal] Remove "UNSUPPORTED: Metal" from some tests #1173

Merged

Conversation

kcloudy0717 commented Apr 6, 2026

Uh oh!

kcloudy0717 commented Apr 6, 2026

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!

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!

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!

manon-traverse left a comment

Choose a reason for hiding this comment

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!

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!

kcloudy0717 commented May 7, 2026

Uh oh!

bogner commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bogner commented May 11, 2026 •

edited

Loading