[webkit-reviews] review granted: [Bug 193877] [WHLSL] Pack and unpack data at entry points and exit points : [Attachment 363790] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 193877: [WHLSL] Pack and unpack data at entry points and exit points
https://bugs.webkit.org/show_bug.cgi?id=193877

Attachment 363790: Patch

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




--- Comment #9 from Dean Jackson <dino at apple.com> ---
Comment on attachment 363790
  --> https://bugs.webkit.org/attachment.cgi?id=363790
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:107
> +	   namedBindGroup.argumentBufferIndex = argumentBufferIndex++;

Why can't you use i here? (or rename i to be argumentBufferIndex)

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:113
> +	       namedBinding.index = index++;

Why can't you use j here?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:407
> +    m_namedOutputs.reserveInitialCapacity(m_entryPointItems.outputs.size());
> +    for (size_t i = 0; i < m_entryPointItems.outputs.size(); ++i) {
> +	   NamedOutput namedOutput;
> +	   namedOutput.elementName =
m_typeNamer.generateNextStructureElementName();
> +	   m_namedOutputs.uncheckedAppend(WTFMove(namedOutput));
> +    }

I wonder if you could use .map for these loops. It sets up the initial capacity
for you.

m_namedOutputs = m_entryPointItems.output.map([] (...) -> NamedOutput {  ....
return namedOutput;  }

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:265
> +    CRASH();

Really? Wouldn't we want to return an invalid program instead?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:360
> +    CRASH();

ditto for other instances of CRASH()

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLSemanticMatcher.cpp:97
> +		   itemIndices.add(i + 1); // Work around the fact that
HashSet's keys are restricted.

Confused--- Can't you use a HashSet with a zero key? You test for max above?