Bug 198433

Summary: [WHLSL] Zero-fill parameters to entry point functions or make any mismatch a compile error
Product: WebKit Reporter: Saam Barati <saam>
Component: WebGPUAssignee: Saam Barati <saam>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dino, fpizlo, jonlee, justin_fan, mmaxfield, rmorisset, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=198440
Attachments:
Description Flags
WIP
none
Patch none

Saam Barati
Reported 2019-05-31 13:53:15 PDT
...
Attachments
WIP (1.38 KB, patch)
2019-05-31 15:48 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
Patch (12.15 KB, patch)
2019-06-21 00:24 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2019-05-31 15:47:06 PDT
Currently, it's a compile error if the JS attributes define fewer things than the WHLSL defined attributes. Maybe we're ok keeping it this way, then we wouldn't need to zero fill
Saam Barati
Comment 2 2019-05-31 15:47:27 PDT
(In reply to Saam Barati from comment #1) > Currently, it's a compile error if the JS attributes define fewer things > than the WHLSL defined attributes. Maybe we're ok keeping it this way, then > we wouldn't need to zero fill If we loosen this, though, we'd need to zero fill. Attaching a patch that does that zero filling.
Saam Barati
Comment 3 2019-05-31 15:48:09 PDT
Created attachment 371093 [details] WIP
Saam Barati
Comment 4 2019-06-19 12:54:03 PDT
We could also make this a compile error. Maybe that's nicer, but I don't think it's a P1
Saam Barati
Comment 5 2019-06-19 12:54:39 PDT
*** Bug 195771 has been marked as a duplicate of this bug. ***
Myles C. Maxfield
Comment 6 2019-06-21 00:24:11 PDT
Created attachment 372620 [details] Patch
Saam Barati
Comment 7 2019-06-21 08:35:07 PDT
Comment on attachment 372620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372620&action=review > LayoutTests/ChangeLog:8 > + Turns out this bug is already fixed, so this just adds some tests. Which part is already fixed? Last time I checked we allow multiple parameters to be hooked up to the same attribute index.
Myles C. Maxfield
Comment 8 2019-06-21 09:37:32 PDT
Comment on attachment 372620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372620&action=review >> LayoutTests/ChangeLog:8 >> + Turns out this bug is already fixed, so this just adds some tests. > > Which part is already fixed? Last time I checked we allow multiple parameters to be hooked up to the same attribute index. https://bugs.webkit.org/show_bug.cgi?id=198440 Is for duplicates. This is for attributes that exist in the shader text but aren’t hooked up to any API objects.
Saam Barati
Comment 9 2019-06-21 11:39:05 PDT
(In reply to Myles C. Maxfield from comment #8) > Comment on attachment 372620 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372620&action=review > > >> LayoutTests/ChangeLog:8 > >> + Turns out this bug is already fixed, so this just adds some tests. > > > > Which part is already fixed? Last time I checked we allow multiple parameters to be hooked up to the same attribute index. > > https://bugs.webkit.org/show_bug.cgi?id=198440 Is for duplicates. This is > for attributes that exist in the shader text but aren’t hooked up to any API > objects. Can you make the changelog more descriptive of what we're testing for here? I'm still confused, because I thought we actually didn't allow code like this? I don't remember ever actually implementing zero filling here.
Saam Barati
Comment 10 2019-06-21 11:39:52 PDT
Comment on attachment 372620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372620&action=review > LayoutTests/webgpu/whlsl-extra-attribute.html:9 > +vertex float4 vertexShader(float4 position : attribute(0), float i : attribute(1)) : SV_Position { Don't you want to be checking that "i" is zero here since it's not hooked up? > LayoutTests/webgpu/whlsl-extra-resource.html:9 > +vertex float4 vertexShader(float4 position : attribute(0), device float[] theBuffer : register(u0)) : SV_Position { Don't you want to be checking that theBuffer is null here?
Radar WebKit Bug Importer
Comment 11 2019-07-05 13:50:45 PDT
<rdar://problem/52699959>
Myles C. Maxfield
Comment 12 2020-05-05 00:42:48 PDT
WHLSL is no longer relevant.
Note You need to log in before you can comment on or make changes to this bug.