Bug 191084 – [WebGPU] Experimental prototype for MSL shaders

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Justin Fan]

[WebGPU] Experimental prototype for MSL shaders and render pipelines

[Justin Fan]

Created attachment 353416 [details]
Patch

[Justin Fan]

Oh yeah. Everything is broken on non-cocoa platforms (and probably ios-sim). Fixing

[Radar WebKit Bug Importer]

<rdar://problem/45683389>

[Dean Jackson]

Comment on attachment 353416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353416&action=review

> Source/WebCore/ChangeLog:10
> +        Begin implementation for WebGPUDevice, WebGPUSwapChain, WebGPUShaderModule, WebGPURenderPipeline,
> +        and any associated descriptor objects. This prototype compiles Metal Shading Language, which will not 
> +        be available for the final implementation.

These should be split up into separate patches. One for creating the shader modules, one for the swap chain, and then one for the render pipeline.

> Source/WebCore/Modules/webgpu/GPUDevice.h:30
> +#include <wtf/Ref.h>

Don't need this.

> Source/WebCore/Modules/webgpu/GPUDevice.h:34
> +OBJC_PROTOCOL(MTLDevice);

Guard with USE(METAL)

> Source/WebCore/Modules/webgpu/GPUDevice.h:41
> +struct WebGPURenderPipelineDescriptor;

Needs to be GPURenderPipelineDescriptor. See below.

> Source/WebCore/Modules/webgpu/GPUDevice.h:43
> +class GPUDevice {

This should be a RefCounted object.

public RefCounted<GPUDevice>

> Source/WebCore/Modules/webgpu/GPUDevice.h:45
> +    GPUDevice();

Make this private. Use a static RefPtr<GPUDevice> create() method to initialise.

> Source/WebCore/Modules/webgpu/GPUDevice.h:48
> +    GPURenderPipeline createRenderPipeline(WebGPURenderPipelineDescriptor&&) const;

This should not take a WebGPU object as a parameter. Everything that is GPUFoo* needs to be ignorant of WebGPU*.

> Source/WebCore/Modules/webgpu/GPUDevice.h:51
> +    MTLDevice *mtlDevice() const { return m_mtlDevice.get(); }

We often call these accessors platformDevice(), and have a typedef for PlatformDevice.

I'm not sure if we should do this right away, since Metal is the only backend for now. It might be a good idea though.

> Source/WebCore/Modules/webgpu/GPUShaderModule.h:33
> +OBJC_PROTOCOL(MTLLibrary);

USE(METAL)

> Source/WebCore/Modules/webgpu/GPUShaderModule.h:39
> +class GPUShaderModule {

RefCounted.

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:51
> +    return WebGPUShaderModule::create(m_device.createShaderModule(descriptor.code));

I feel like the shader module should take the entire descriptor.

> Source/WebCore/Modules/webgpu/WebGPUDevice.h:62
> +    GPUDevice m_device;

This becomes a RefPtr<GPUDevice>

> Source/WebCore/Modules/webgpu/WebGPUDevice.idl:55
> +*/
>      WebGPUShaderModule createShaderModule(WebGPUShaderModuleDescriptor descriptor);
> +/*
>      WebGPUAttachmentState createAttachmentState(WebGPUAttachmentStateDescriptor descriptor);
>      WebGPUComputePipeline createComputePipeline(WebGPUComputePipelineDescriptor descriptor);
> +*/
>      WebGPURenderPipeline createRenderPipeline(WebGPURenderPipelineDescriptor descriptor);
> -
> +/*

I suggest keeping the unused objects in a group, rather than commenting bits and pieces out. We can clean up the finished IDL later to look like the official one.

> Source/WebCore/Modules/webgpu/WebGPUPipelineDescriptorBase.h:37
> +struct WebGPUPipelineDescriptorBase {
> +    Vector<WebGPUPipelineStageDescriptor> stages;
> +};

This is the base class. I wonder if it needs to be RefCounted. We do hold onto them.

> Source/WebCore/Modules/webgpu/WebGPUPipelineDescriptorBase.idl:28
> +    // FIXME: Figure out what PipelineLayout is for.

Remove this comment.

> Source/WebCore/Modules/webgpu/WebGPURenderPipeline.h:44
> +    GPURenderPipeline m_renderPipeline;

Becomes Ref<>

> Source/WebCore/Modules/webgpu/WebGPURenderPipelineDescriptor.idl:42
> +    /*
> +    sequence<WebGPUBlendState> blendStates;
> +    WebGPUDepthStencilState depthStencilState;
> +    WebGPUInputState inputState;
> +    WebGPUAttachmentsState attachmentsState; */
> +    // TODO other properties

Make this more clear that they are not implemented. Put // in front rather than a block comment.

> Source/WebCore/Modules/webgpu/WebGPUShaderStage.h:36
> +    enum : unsigned long {

Even though this is a u32 in the IDL, it doesn't need to be in the implementation.

> Source/WebCore/Modules/webgpu/WebGPUSwapChain.h:40
> +        // FIXME: More texture properties.

Remove this, or link to a bug.

> Source/WebCore/Modules/webgpu/WebGPUSwapChain.h:69
> +    GPUSwapChain m_swapChain;

Needs to be a Ref or RefPtr.

> Source/WebCore/Modules/webgpu/cocoa/GPUDeviceMetal.mm:41
> +    : m_mtlDevice { adoptNS(MTLCreateSystemDefaultDevice()) }

I would prefer this go in the function body. Eventually it won't be a call to creating the default device.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipeline.h:40
> +class GPURenderPipeline {

Should be RefCounted.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipeline.h:42
> +    GPURenderPipeline(const GPUDevice&, WebGPURenderPipelineDescriptor&&);

See above about disconnecting from WebGPU objects.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:41
> +        return; // FIXME: Throw relevant error before returning.

I think we should do this now. Alas, this probably means a bit of code to throw exceptions from the WebGPURenderPipeline.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:48
> +        // FIXME: How many encapsulation laws does this break??

All of them :)

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:49
> +        auto mtlLibrary = retainPtr(stageDescriptor.module->m_module.mtlLibrary());

If we access the mtlLibrary this way, the accessor might as well give us a RetainPtr<> directly.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:52
> +        if (!mtlLibrary)
> +            return; // FIXME: Throw relevant error before returning.

Yeah. This means the shader didn't compile.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:56
> +            [mtlDescriptor setVertexFunction:[mtlLibrary newFunctionWithName:stageDescriptor.entryPoint]];

We need to throw an error if we were unable to find the function, otherwise we simply crash when we go to draw.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:67
> +    // FIXME: Get the pixelFormat as configured for the context/CAMetalLayer.

File a bug and link to it here.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:70
> +    // FIXME: Assert that both vertex and fragment functions have been assigned?

Yes.

> Source/WebCore/Modules/webgpu/cocoa/GPUShaderModuleMetal.mm:45
> +    NSError *error = [NSError errorWithDomain:@"com.apple.WebKit.GPU" code:1 userInfo:nil];
> +    m_mtlLibrary = adoptNS([device.mtlDevice() newLibraryWithSource:code options:nil error:&error]);
> +    if (!m_mtlLibrary)
> +        LOG(WebGPU, "Shader compilation error: %s", [[error localizedDescription] UTF8String]);

Again, we need to handle this properly, otherwise we'll crash horrifically :)

Maybe the IDL doesn't yet expose a way to throw an exception, but we should add one.

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChain.h:32
> +OBJC_CLASS CAMetalLayer;

USE(METAL)

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChain.h:42
> +    void setDevice(const GPUDevice&);

This should take a Ref<>. I'm not sure how the ownership will happen - I guess the WebGPUSwapChain can go away and break the ref.

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChainMetal.mm:38
> +    : m_layer(adoptNS([[CAMetalLayer alloc] init]))

Move into function body.

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChainMetal.mm:43
> +    // FIXME: For now, default to these settings.

File a bug and link to it.

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChainMetal.mm:51
> +    if (!device.mtlDevice())
> +        return; // FIXME: Throw appropriate error

We should do this now.

> LayoutTests/ChangeLog:10
> +        * webgpu/webgpu-basics.html: Added.

You should make new test files to exercise creating the render pipeline and shader modules.

> LayoutTests/webgpu/webgpu-basics.html:56
> +    window.webgpu.requestAdapter({ powerPreference: "default" }).then(adapter_ => {

You can remove this { powerPreference } parameter.

[Justin Fan]

Created attachment 353547 [details]
Patch

[EWS Watchlist]

Attachment 353547 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/GPUShaderModule.h:48:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/webgpu/GPUDevice.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 353547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353547&action=review

> Source/WTF/ChangeLog:3
> +        [WebGPU] Experimental prototype for MSL shaders and render pipelines

This title is now wrong.

> Source/WebCore/CMakeLists.txt:-456
> -    Modules/webgpu/WebGPUSwapChainDescriptor.idl

Did this file never exist? I don't understand why it is being removed in this patch.

> Source/WebCore/Modules/webgpu/GPUDevice.h:44
> +using PlatformDevice =
> +#if USE(METAL)
> +    MTLDevice;
> +#else
> +    void;
> +#endif

Our convention is that the entire statement goes in the #if section. So

#if USE(METAL)
using PlatformDevice = MTLDevice;
...

> Source/WebCore/Modules/webgpu/GPUDevice.h:54
> +    Ref<GPUShaderModule> createShaderModule(GPUShaderModuleDescriptor&&) const;

Can this ever fail? I think it can if the descriptor is bogus, so it might have to be a RefPtr.

> Source/WebCore/Modules/webgpu/GPUDevice.h:61
> +    RetainPtr<PlatformDevice> m_platformDevice;

I guess the issue here is that it is only a RetainPtr in USE(METAL) (where it is an ObjC object). Maybe guard this with #if USE(METAL) as well for now.

> Source/WebCore/Modules/webgpu/GPUShaderModule.h:49
> +using PlatformShaderModule =
> +#if USE(METAL)
> +    MTLLibrary;
> +#else
> +    void;
> +#endif

Same as above. Although you can probably put the OBJC_PROTOCOL bit in here too.

> Source/WebCore/Modules/webgpu/GPUShaderModule.h:60
> +    RetainPtr<PlatformShaderModule> m_platformShaderModule;

And ditto with the RetainPtr.

> Source/WebCore/Modules/webgpu/cocoa/GPUDeviceMetal.mm:57
> +Ref<GPUDevice> GPUDevice::create()
> +{
> +    return adoptRef(*new GPUDevice());
> +}

We put this first in the .cpp file.

> Source/WebCore/Modules/webgpu/cocoa/GPUShaderModuleMetal.mm:57
> +Ref<GPUShaderModule> GPUShaderModule::create(const GPUDevice& device, GPUShaderModuleDescriptor&& descriptor)
> +{
> +    return adoptRef(*new GPUShaderModule(device, WTFMove(descriptor)));
> +}

Same here. Goes first.

Also, I think we need to do something if there is no device - return nullptr. At the moment you create what looks like a valid GPUShaderModule.

> LayoutTests/ChangeLog:8
> +        * webgpu/webgpu-basics.html: Added.

This should be called shadermodule-basics.html or something like that.

> LayoutTests/webgpu/webgpu-basics.html:72
> +    window.webgpu.requestAdapter().then(adapter_ => {
> +        if (!adapter_) {
> +            testFailed("Could not create default WebGPUAdapter!")
> +            return;
> +        }
> +        adapter = adapter_;
> +
> +        defaultDevice = adapter.createDevice();
> +        if (!defaultDevice) {
> +            testFailed("Could not create WebGPUDevice!");
> +            return;
> +        }
> +
> +        setUpShaders();
> +    }, error => {
> +        console.log(error);
> +    });

This code is fine, but I tend to find async/await a bit easier to read.

The function would become async function setUpContexts()

And then the body would be:

adapter = await window.webgpu.requestAdapter();
if (!adapter)...

If you still think there might be an error, you put it in try catch.

> LayoutTests/webgpu/webgpu-basics.html:82
> +        return;

This return isn't needed.

[Justin Fan]

(In reply to Dean Jackson from comment #7)
> Comment on attachment 353547 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353547&action=review

WebGPUSwapChainDescriptor's contents were merged into the WebGPUSwapChain class since it isn't referenced in any other IDL.

webgpu-basics will contain more functionality all up until drawing a triangle as features are implemented, but I'll whip up some more tests to stress each new feature.

[Justin Fan]

Created attachment 353669 [details]
Patch

[EWS Watchlist]

Attachment 353669 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 353669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353669&action=review

We should probably add a test with bad shader code to ensure our promise is rejected.

I think the sooner you make small tests for each bit of the library, the better.

> Source/WebCore/ChangeLog:3
> +        [WebGPU] Experimental prototype for MSL shaders and render pipelines

Needs to be accurate.

> Source/WebCore/Modules/webgpu/GPUDevice.h:34
> +#if ENABLE(WEBGPU)

Typo: METAL

> Source/WebCore/Modules/webgpu/WebGPUDevice.h:46
> +    static RefPtr<WebGPUDevice> create(ScriptExecutionContext&, Ref<WebGPUAdapter>&&);

Why do we need the ScriptExecutionContext?

> Source/WebCore/Modules/webgpu/cocoa/GPUDeviceMetal.mm:68
> +GPUDevice::GPUDevice(PlatformDeviceSmartPtr&& device)
> +    : m_platformDevice(WTFMove(device))
> +{
> +    UNUSED_PARAM(m_platformDevice);
> +}
> +
> +RefPtr<GPUShaderModule> GPUDevice::createShaderModule(GPUShaderModuleDescriptor&& descriptor) const
> +{
> +    return GPUShaderModule::create(*this, WTFMove(descriptor));
> +}

Technically these bits could go in a GPUDevice.cpp file, but we can do that later.

[Justin Fan]

Created attachment 353674 [details]
Patch

[EWS Watchlist]

Attachment 353674 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 353674 [details]
Patch

Clearing flags on attachment: 353674

Committed r237723: <https://trac.webkit.org/changeset/237723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.