Bug 195446 – [WHLSL] Vertex shader and fragment shader need to be able to come from two different programs

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

[Myles C. Maxfield]

The WebGPU API lets you take one shader from one program and another shader from another program.

[Myles C. Maxfield]

This bug probably includes implementing two-phase compilation: one in WebGPUDevice.createShaderModule() and one in WebGPUDevice.createRenderPipeline().

[Radar WebKit Bug Importer]

<rdar://problem/50746306>

[Myles C. Maxfield]

Created attachment 376648 [details]
WIP

[Myles C. Maxfield]

Created attachment 376655 [details]
WIP

[Myles C. Maxfield]

Created attachment 376657 [details]
Needs tests

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 19 files


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

[Myles C. Maxfield]

Created attachment 376659 [details]
Needs tests

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 23 files


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

[Myles C. Maxfield]

Created attachment 376676 [details]
WIP

[Myles C. Maxfield]

Created attachment 376678 [details]
WIP

[Myles C. Maxfield]

Created attachment 376681 [details]
Patch

[Saam Barati]

Comment on attachment 376681 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:38
> +        to find all it's potential overloads. These buckets can't be educated about namespaces (consider a

it's => its

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:164
> +        if (static_cast<bool>(m_castReturnType) != static_cast<bool>(other.m_castReturnType))

🤔

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:675
> +    HashSet<String> m_vertexEntryPoints[3];
> +    HashSet<String> m_fragmentEntryPoints[3];
> +    HashSet<String> m_computeEntryPoints[3];

let's use a constant for this like "numberOfNamespaces" instead of 3.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCodeLocation.h:67
> +    unsigned startOffset() const
> +    {
> +        return m_startOffset & 0x7FFFFFFF;
> +    }
> +    unsigned endOffset() const
> +    {
> +        return m_endOffset & 0x7FFFFFFF;
> +    }
> +    AST::NameSpace nameSpace() const
> +    {
> +        unsigned result = (m_startOffset & 0x80000000) >> 31;
> +        result |= (m_endOffset & 0x80000000) >> 30;
> +        return static_cast<AST::NameSpace>(result);
> +    }

why not just use a bitfield in this class instead?

unsigned m_startOffset : 31;
unsigned m_endOffset : 31;
unsigned m_nameSpace : 2;

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameContext.cpp:218
> +AST::FunctionDeclaration* NameContext::searchFunctions(String& name) const

can we cache the results of this? I'm guessing this is much more expensive than it used to be?

Maybe we can cache results per namespace?

Or we can add standard library to both user namespaces and use the old implementation

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameContext.cpp:243
> +Optional<CodeLocation> NameContext::globalExists(String& name) const

my initial reaction to this name was that it transcends namespaces. Can we just call it "exists"  or something like that? And just assert that we're the top-most?

Or maybe "typeOrFunctionExists" with the ASSERT? Then call the "localExists" below "variableExists" or "getVariable" or "variable"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.h:46
> +    Expected<void, Error> parse(Program&, StringView, ParsingMode, AST::NameSpace = AST::NameSpace::StandardLibrary);

small nit: do we really need this as a default argument? Seems like it'd be better to make sure every caller of this knows what they're doing.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:200
> +                // FIXME: The first argument to errorString()

I don't understand what this means. Same with the above FIXMEs which are the same

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.h:47
> +using OwnedShaderModule = std::unique_ptr<ShaderModule, ShaderModuleDeleter>;

nit: why not UniqueRef?

(You can specify a deleter by changing the default_delete in namespace std for ShaderModule. Or, you can change UniqueRef to explicitly take a Deleter argument and have it default to default_delete)

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibraryUtilities.cpp:86
> +        if (!parseResult)
> +            return makeUnexpected(parseResult.error());

why?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibraryUtilities.cpp:95
> +    if (!parseResult)
> +        return makeUnexpected(parseResult.error());

why?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibraryUtilities.cpp:105
> +    // We need to make sure that an author can't write a function with the same signature as anything in the standard library.
> +    // If they do so, we need to make sure it collides.

This comment makes me think this work is happening here. Maybe add another sentence saying we include them here so later phases can do this check?

[Myles C. Maxfield]

This causes a 16% regression :(

[Saam Barati]

(In reply to Myles C. Maxfield from comment #13)
> This causes a 16% regression :(

In its current form or with a reworked getFunctions?

[Myles C. Maxfield]

Comment on attachment 376681 [details]
Patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibraryUtilities.cpp:86
>> +            return makeUnexpected(parseResult.error());
> 
> why?

We parse the user program first, and then we parse the standard library. Inside the parser, after we've successfully parsed a struct, for example, we add it to the Program, which checks to see if the name isn't a duplicate. If the name is a duplicate, the "add" function returns failure, which makes the parser return failure. So, if there is a duplicate struct, the way it manifests is parsing the standard library fails at the site of the duplicate.

[Myles C. Maxfield]

(In reply to Saam Barati from comment #14)
> (In reply to Myles C. Maxfield from comment #13)
> > This causes a 16% regression :(
> 
> In its current form or with a reworked getFunctions?

In its current form.

[Myles C. Maxfield]

Created attachment 376958 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:81:  default_delete::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 85 files


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

[Myles C. Maxfield]

Turns out I don't need to fix perf. The number I quoted earlier is from 1 trial. I ran 100 trials and found a 0.3% regression, and a p value of 0.1991, which means this is not statistically significant.

[WebKit Commit Bot]

Comment on attachment 376958 [details]
Patch

Clearing flags on attachment: 376958

Committed r248993: <https://trac.webkit.org/changeset/248993>