[WebGPU] Support tier1 level argument buffers #8275
Conversation
|
EWS run on previous version of this PR (hash e9ea635)
|
| @@ -109,21 +109,28 @@ static bool isPresent(const WGPUStorageTextureBindingLayout& storageTexture) | |||
| return nil; | |||
|
|
|||
| UNUSED_PARAM(storageTexture); | |||
| auto descriptor = [MTLArgumentDescriptor new]; | |||
| auto descriptor = [MTLArgumentDescriptor argumentDescriptor]; | |||
There was a problem hiding this comment.
This change isn't needed, it could be reverted.
webkit-ews-buildbot
added
the
merging-blocked
mwyrzykowski
removed
the
merging-blocked
mwyrzykowski
force-pushed
the
eng/WebGPU-Support-tier1-level-argument-buffers
branch
from
e9ea635
to
ca18423
Compare
|
EWS run on previous version of this PR (hash ca18423)
|
| @@ -41,13 +41,9 @@ class Device; | |||
| class BindGroupLayout : public WGPUBindGroupLayoutImpl, public RefCounted<BindGroupLayout> { | |||
| WTF_MAKE_FAST_ALLOCATED; | |||
| public: | |||
| static Ref<BindGroupLayout> create(id<MTLArgumentEncoder> vertexArgumentEncoder, id<MTLArgumentEncoder> fragmentArgumentEncoder, id<MTLArgumentEncoder> computeArgumentEncoder) | |||
| static Ref<BindGroupLayout> create(HashMap<uint32_t, WGPUShaderStageFlags>&& stageMapTable, id<MTLArgumentEncoder> vertexArgumentEncoder = nil, id<MTLArgumentEncoder> fragmentArgumentEncoder = nil, id<MTLArgumentEncoder> computeArgumentEncoder = nil) | |||
There was a problem hiding this comment.
Why not pass as an array of id<MTLArgumentEncoder>s instead of individually? (And instead of m_vertexArgumentEncoder, access would be via m_argumentEncoder[vertex])
There was a problem hiding this comment.
It is possible but I would need to make vertex / fragment / compute accessible via the BindGroupLayout too, currently they are only defined in BindGroup.mm
mwyrzykowski
force-pushed
the
eng/WebGPU-Support-tier1-level-argument-buffers
branch
from
ca18423
to
8277c6f
Compare
|
EWS run on previous version of this PR (hash 8277c6f) |
mwyrzykowski
force-pushed
the
eng/WebGPU-Support-tier1-level-argument-buffers
branch
from
8277c6f
to
d06450e
Compare
|
EWS run on previous version of this PR (hash d06450e) |
| Vector<BindableResource> resources; | ||
| #if HAVE(TIER2_ARGUMENT_BUFFERS) |
Source/WebGPU/WebGPU/BindGroup.mm
Outdated
| Ref<BindGroup> Device::createBindGroup(const WGPUBindGroupDescriptor& descriptor) | ||
| { | ||
| if (descriptor.nextInChain) | ||
| return BindGroup::createInvalid(*this); | ||
|
|
||
| constexpr WGPUShaderStage shaderStage[] = { WGPUShaderStage_Vertex, WGPUShaderStage_Fragment, WGPUShaderStage_Compute }; |
There was a problem hiding this comment.
I'd like your opinion about something. On a previous review, Kimmo suggested considering the WGPU types as external types, only to be used at the API boundary of the framework. Just like how we have WGPUBuffer as an API type, but internally we have WebGPU::Buffer, we could extend that relationship to all WGPU types. The "binding" code to turn all WGPU types into their corresponding WebGPU:: types would be generated code, driven by IDL as a custom build step.
What do you think about the tradeoff of having cleaner internal/external boundaries, vs the added infrastructure (and potential runtime cost?) of transforming a bunch of types into different types that have the same expressivity?
There was a problem hiding this comment.
I think this makes a lot of sense since if the external (WGPU) types change in any form (renames or values change), we do not want to have a lot of compilation or runtime breakage throughout our code.
There was a problem hiding this comment.
I'll make this change prior to merging
Source/WebGPU/WebGPU/BindGroup.mm
Outdated
|
|
||
| return BindGroup::create(argumentBuffer[0], argumentBuffer[1], argumentBuffer[2], WTFMove(resources), *this); | ||
| id<MTLArgumentEncoder> argumentEncoder[] = { bindGroupLayout.vertexArgumentEncoder(), bindGroupLayout.fragmentArgumentEncoder(), bindGroupLayout.computeArgumentEncoder() }; | ||
| for (size_t i = 0; i < shaderStageLength; ++i) { |
There was a problem hiding this comment.
How do you feel about naming these values of i in an enum, and doing this in a lambda instead of a loop? EnumeratedArray is a pretty elegant thing to use for situations like these.
enum class ShaderType {
Vertex,
Fragment,
Compute
};
EnumeratedArray<ShaderType, id<MTLArgumentEncoder>, ShaderType::Compute> argumentEncoders;
auto foo = [](ShaderType type) {
... argumentEncoders[type] ...
}
foo(ShaderType::Vertex);
foo(ShaderType::Fragment);
foo(ShaderType::Compute);
There was a problem hiding this comment.
I think using EnumeratedArray makes sense, I'm attempting to apply the suggestion of using EnumeratedArray without using the lambdas since it doubles the code lines of the first for loop which I think impacts readability.
Source/WebGPU/WebGPU/BindGroup.mm
Outdated
| ASSERT_NOT_REACHED(); | ||
| return BindGroup::createInvalid(*this); | ||
| auto stages = bindGroupLayout.stagesForBinding(entry.binding); | ||
| for (size_t currentStage = 0; currentStage < shaderStageLength; ++currentStage) { |
There was a problem hiding this comment.
(ditto, if you decide that it's a good idea)
Source/WebGPU/WebGPU/BindGroup.mm
Outdated
| if (bufferIsPresent) { | ||
| id<MTLBuffer> buffer = WebGPU::fromAPI(entry.buffer).buffer(); | ||
| if (entry.offset > buffer.length) | ||
| return BindGroup::createInvalid(*this); | ||
|
|
||
| [argumentEncoder[currentStage] setBuffer:buffer offset:entry.offset atIndex:index++]; | ||
| } else if (samplerIsPresent) { | ||
| id<MTLSamplerState> sampler = WebGPU::fromAPI(entry.sampler).samplerState(); | ||
| [argumentEncoder[currentStage] setSamplerState:sampler atIndex:index++]; | ||
| } else if (textureViewIsPresent) { | ||
| id<MTLTexture> texture = WebGPU::fromAPI(entry.textureView).texture(); | ||
| [argumentEncoder[currentStage] setTexture:texture atIndex:index++]; | ||
| resources.append({ texture, MTLResourceUsageRead, renderStage }); | ||
| } |
Source/WebGPU/WebGPU/BindGroup.mm
Outdated
| } | ||
| } | ||
|
|
||
| return BindGroup::create(vertexArgumentBuffer, fragmentArgumentBuffer, computeArgumentBuffer, WTFMove(resources), *this); | ||
| constexpr int vertex = 0, fragment = 1, compute = 2; |
There was a problem hiding this comment.
Right, the enum would save you from having to do this
| @@ -488,6 +488,32 @@ static MTLVertexStepFunction stepFunction(WGPUVertexStepMode stepMode) | |||
|
|
|||
| RenderPipeline::~RenderPipeline() = default; | |||
|
|
|||
| #if HAVE(METAL_BUFFER_BINDING_REFLECTION) | |||
mwyrzykowski
force-pushed
the
eng/WebGPU-Support-tier1-level-argument-buffers
branch
from
d06450e
to
16113e0
Compare
|
EWS run on previous version of this PR (hash 16113e0) |
mwyrzykowski
force-pushed
the
eng/WebGPU-Support-tier1-level-argument-buffers
branch
from
16113e0
to
d188141
Compare
|
EWS run on current version of this PR (hash d188141) |
https://bugs.webkit.org/show_bug.cgi?id=250123 <radar://103905011> Reviewed by Myles C. Maxfield. Support Tier1 argument buffers which are limited to pre-A13 devices and some Intel Macs. Tested on a pre-A13 iPad Pro. * Source/WebGPU/WebGPU/BindGroup.mm: (WebGPU::sizeOfEntries): (WebGPU::Device::createBindGroup): Support tier1 ABs. * Source/WebGPU/WebGPU/BindGroupLayout.h: (WebGPU::BindGroupLayout::create): Simplify create function. * Source/WebGPU/WebGPU/BindGroupLayout.mm: (WebGPU::createArgumentDescriptor): (WebGPU::addDescriptor): (WebGPU::Device::createBindGroupLayout): (WebGPU::BindGroupLayout::BindGroupLayout): (WebGPU::BindGroupLayout::encodedLength const): Support tier1 ABs. * Source/WebGPU/WebGPU/RenderPipeline.mm: (WebGPU::createEntryFromStructMember): (WebGPU::RenderPipeline::getBindGroupLayout): Call Device::createBindLayout instead and pass some additional information to be compatible with Tier1 ABs. Canonical link: https://commits.webkit.org/258651@main
webkit-commit-queue
force-pushed
the
eng/WebGPU-Support-tier1-level-argument-buffers
branch
from
d188141
to
8e36095
Compare
|
Committed 258651@main (8e36095): https://commits.webkit.org/258651@main Reviewed commits have been landed. Closing PR #8275 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
8e36095
d188141
🧪 api-ios