Bug 217095 – [WebGPU] Add platform guards

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

[Don Olmstead]

Compiling WebGPU currently assumes Metal is present

[Don Olmstead]

Created attachment 410016 [details]
Patch

[Don Olmstead]

Created attachment 410021 [details]
Patch

[Don Olmstead]

Created attachment 410022 [details]
Patch

[Don Olmstead]

Created attachment 410026 [details]
Patch

[Darin Adler]

Comment on attachment 410026 [details]
Patch

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

Not totally thrilled with details of the approach. Happy that we are adding the #if USE(METAL).

> Source/WebCore/platform/graphics/gpu/GPUBindGroupAllocator.h:40
> +#if USE(METAL)
> +#include <objc/NSObjCRuntime.h>
>  #include <wtf/RetainPtr.h>
>  
>  OBJC_PROTOCOL(MTLArgumentEncoder);
>  OBJC_PROTOCOL(MTLBuffer);
> +#endif

I’d prefer we use two separate #if/#endif pairs here. Same feedback over and over again. Nicer to have all includes together, then all forward declarations together, rather than trying to group into platforms.

> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:62
>  #else
> -using PlatformBuffer = void;
> +#error Unsupported configuration

I think we’ll get errors without an explicit #error, so maybe don’t bother with the #else.

Not the biggest fan of platform-specifics with a chain of #if/#elif/#endif. Seems like we can just define these types separately as needed.

But more, generally speaking, I am not so fond of these types; how much platform-independent code to handle platform-specific types really needs to exist at all? I’d rather see #if around multiple definitions of m_platformBuffer, because I don’t really see this as a platform-independent abstraction. Very little platform-independent code needs to deal with these; the platform-specific code does need to.

One exception is the constructor, I guess?

We can do better.

Same comment repeatedly about other cases below.

> Source/WebCore/platform/graphics/gpu/GPUSwapChain.h:35
> +// PlatformLayer implementation needed otherwise compiling derived sources will fail

Please add a period.

[Dean Jackson]

Comment on attachment 410026 [details]
Patch

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

>> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:62
>> +#error Unsupported configuration
> 
> I think we’ll get errors without an explicit #error, so maybe don’t bother with the #else.
> 
> Not the biggest fan of platform-specifics with a chain of #if/#elif/#endif. Seems like we can just define these types separately as needed.
> 
> But more, generally speaking, I am not so fond of these types; how much platform-independent code to handle platform-specific types really needs to exist at all? I’d rather see #if around multiple definitions of m_platformBuffer, because I don’t really see this as a platform-independent abstraction. Very little platform-independent code needs to deal with these; the platform-specific code does need to.
> 
> One exception is the constructor, I guess?
> 
> We can do better.
> 
> Same comment repeatedly about other cases below.

+1 to what Darin said. I find the PlatformType stuff fairly confusing, because it means you have to redirect to work out what the actual type is.

[Don Olmstead]

Created attachment 410071 [details]
Patch

[Don Olmstead]

(In reply to Dean Jackson from comment #6)
> Comment on attachment 410026 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410026&action=review
> 
> >> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:62
> >> +#error Unsupported configuration
> > 
> > I think we’ll get errors without an explicit #error, so maybe don’t bother with the #else.
> > 
> > Not the biggest fan of platform-specifics with a chain of #if/#elif/#endif. Seems like we can just define these types separately as needed.
> > 
> > But more, generally speaking, I am not so fond of these types; how much platform-independent code to handle platform-specific types really needs to exist at all? I’d rather see #if around multiple definitions of m_platformBuffer, because I don’t really see this as a platform-independent abstraction. Very little platform-independent code needs to deal with these; the platform-specific code does need to.
> > 
> > One exception is the constructor, I guess?
> > 
> > We can do better.
> > 
> > Same comment repeatedly about other cases below.
> 
> +1 to what Darin said. I find the PlatformType stuff fairly confusing,
> because it means you have to redirect to work out what the actual type is.

When I'm going through the actual implementation with Dawn I'll revisit the PlatformType stuff.

[EWS]

Committed r267777: <https://trac.webkit.org/changeset/267777>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410071 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69773470>