[Metal] Explicit root signature layout for Metal backend#1061
[Metal] Explicit root signature layout for Metal backend#1061kcloudy0717 wants to merge 5 commits intollvm:mainfrom
Conversation
|
Pinging @bogner, @bob80905, @farzonl, @manon-traverse for review. |
lib/API/MTL/MTLDescriptorHeap.cpp
Outdated
| #define IR_RUNTIME_METALCPP | ||
| #include "MTLDescriptorHeap.h" | ||
| #include "metal_irconverter_runtime.h" |
There was a problem hiding this comment.
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)
| #include "llvm/Support/Error.h" | ||
| #include <memory> | ||
|
|
||
| struct IRDescriptorTableEntry; |
There was a problem hiding this comment.
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.
lib/API/MTL/MTLDescriptorHeap.h
Outdated
| struct IRDescriptorTableEntry; | ||
|
|
||
| namespace offloadtest { | ||
| struct METAL_GPU_DESCRIPTOR_HANDLE { |
There was a problem hiding this comment.
Let's stick to LLVM naming conventions for these types (ie, MetalGPUDescriptorHandle)
lib/API/MTL/MTLDescriptorHeap.h
Outdated
| enum METAL_DESCRIPTOR_HEAP_TYPE { | ||
| METAL_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV, | ||
| METAL_DESCRIPTOR_HEAP_TYPE_SAMPLER, | ||
| }; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
|
||
| #include "MTLDescriptorHeap.h" | ||
| #include "Metal/Metal.hpp" | ||
| #include "metal_irconverter.h" |
There was a problem hiding this comment.
Do we need all of this for the header?
There was a problem hiding this comment.
Yes, but I think I can get away with forward declarations for some of them. I'll do that.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What would be the preferred approach? Maybe we can use CMake Fetch_Content to make this bit simpler?
There was a problem hiding this comment.
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.
| SampledTexture2D, | ||
| }; | ||
|
|
||
| enum class DescriptorKind { UAV, SRV, CBV, SAMPLER }; |
There was a problem hiding this comment.
I am not sure if we should expose this here. This is a concept that only makes sense for DX12 and Metal, but not for Vulkan. I do see the value of deduplicating this code between DX and Metal. Perhaps we should put this in some sort of util header file?
| // 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 |
There was a problem hiding this comment.
This seems fine to me as Metal is Apple only.
3 participants
Resolves #351, #744, #305.
This PR changes the automatic linear layout binding model to the more flexible explicit root signature binding model.
Notable changes:
-metal&&-Freto 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.MTLTopLevelArgumentBuffer(TLAB) class that replaces the legacyMTL::Buffer *ArgsBufferinInvocationState. 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.MTLDescriptorHeapclass 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.MTLTopLevelArgumentBufferandMTLDescriptorHeap, this allows resource arrays to be supported in the Metal backend now.