[webkit-reviews] review granted: [Bug 195446] [WHLSL] Vertex shader and fragment shader need to be able to come from two different programs : [Attachment 376681] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 195446: [WHLSL] Vertex shader and fragment shader need to be able to come
from two different programs
https://bugs.webkit.org/show_bug.cgi?id=195446

Attachment 376681: Patch

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




--- Comment #12 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 376681
  --> https://bugs.webkit.org/attachment.cgi?id=376681
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?