Bug 193877 - [WHLSL] Pack and unpack data at entry points and exit points
Summary: [WHLSL] Pack and unpack data at entry points and exit points
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-26 17:27 PST by Myles C. Maxfield
Modified: 2019-03-06 15:09 PST (History)
4 users (show)

See Also:


Attachments
WIP (64.23 KB, patch)
2019-01-26 17:29 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (64.23 KB, patch)
2019-01-27 10:02 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (125.19 KB, patch)
2019-01-27 22:45 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (125.26 KB, patch)
2019-03-05 14:26 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (127.73 KB, patch)
2019-03-06 14:03 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-01-26 17:27:55 PST
[WHLSL] Pack and unpack data at entry points and exit points
Comment 1 Myles C. Maxfield 2019-01-26 17:29:37 PST
Created attachment 360258 [details]
WIP
Comment 2 EWS Watchlist 2019-01-26 17:32:06 PST
Attachment 360258 [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 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Myles C. Maxfield 2019-01-27 10:02:33 PST
Created attachment 360298 [details]
Patch
Comment 4 EWS Watchlist 2019-01-27 10:03:39 PST
Attachment 360298 [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 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Myles C. Maxfield 2019-01-27 22:45:34 PST
Created attachment 360319 [details]
Patch
Comment 6 EWS Watchlist 2019-01-27 22:48:35 PST
Attachment 360319 [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 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Myles C. Maxfield 2019-03-05 14:26:46 PST
Created attachment 363689 [details]
WIP
Comment 8 Myles C. Maxfield 2019-03-06 14:03:56 PST
Created attachment 363790 [details]
Patch
Comment 9 Dean Jackson 2019-03-06 14:24:21 PST
Comment on attachment 363790 [details]
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?
Comment 10 WebKit Commit Bot 2019-03-06 15:08:58 PST
Comment on attachment 363790 [details]
Patch

Clearing flags on attachment: 363790

Committed r242571: <https://trac.webkit.org/changeset/242571>
Comment 11 WebKit Commit Bot 2019-03-06 15:09:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-03-06 15:09:20 PST
<rdar://problem/48654341>