WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
177303
WSL should have some post-instantiation type checking
https://bugs.webkit.org/show_bug.cgi?id=177303
Summary
WSL should have some post-instantiation type checking
Filip Pizlo
Reported
2017-09-21 08:50:45 PDT
There are certain things that are easier to check after type instantiation. Shader type checking is definitely in that category.
Attachments
the patch
(30.39 KB, patch)
2017-09-21 09:37 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
the patch
(30.54 KB, patch)
2017-09-21 10:45 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
the patch
(30.86 KB, patch)
2017-09-21 10:53 PDT
,
Filip Pizlo
keith_miller
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Filip Pizlo
Comment 1
2017-09-21 09:37:47 PDT
Created
attachment 321439
[details]
the patch
Filip Pizlo
Comment 2
2017-09-21 10:45:06 PDT
Created
attachment 321447
[details]
the patch
Filip Pizlo
Comment 3
2017-09-21 10:53:54 PDT
Created
attachment 321450
[details]
the patch
Filip Pizlo
Comment 4
2017-09-21 14:15:27 PDT
***
Bug 177098
has been marked as a duplicate of this bug. ***
Keith Miller
Comment 5
2017-09-21 14:28:55 PDT
Comment on
attachment 321450
[details]
the patch View in context:
https://bugs.webkit.org/attachment.cgi?id=321450&action=review
r=me with some nits.
> Tools/ChangeLog:26 > + Finally, this removes the shader type checker's incomplete reimplementation of isPrimitive. That calls > + isPrimitive now.
I don't understand what this is saying. What is "That"? isPrimitive was/is a property. AFAICT, there is no isPrimitive function?
> Tools/WebGPUShadingLanguageRI/LateChecker.js:31 > + constructor() > + { > + super(); > + }
Nit: you can remove this. Unless you want to be really sure that no arguments are passed to Visitor.
> Tools/WebGPUShadingLanguageRI/LateChecker.js:52 > + if (!(node.returnType.type instanceof StructType)) > + throw new WTypeError(node.origin.originString, "Vertex shader " + node.name + " must return a struct."); > + assertPrimitive(node.returnType);
Nit: you could probably move this and the one below out of the switch.
Filip Pizlo
Comment 6
2017-09-21 14:33:03 PDT
(In reply to Keith Miller from
comment #5
)
> Comment on
attachment 321450
[details]
> the patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=321450&action=review
> > r=me with some nits. > > > Tools/ChangeLog:26 > > + Finally, this removes the shader type checker's incomplete reimplementation of isPrimitive. That calls > > + isPrimitive now. > > I don't understand what this is saying. What is "That"? isPrimitive was/is a > property. AFAICT, there is no isPrimitive function?
There is an isPrimitive getter all over the place. For example: let assertPrimitive = type => { if (!type.isPrimitive) throw new WTypeError(node.origin.originString, "Shader signature cannot include non-primitive type: " + type); } Before, this code used its own probably-mostly-correct reimplementation: class NonNumericSearcher extends Visitor { constructor(name) { super(); this._name = name; } visitArrayRefType(node) { throw new WTypeError(node.origin.originString, shaderFunc.name + " must transitively only have numeric types."); } visitPtrType(node) { throw new WTypeError(node.origin.originString, shaderFunc.name + " must transitively only have numeric types."); } visitTypeRef(node) { super.visitTypeRef(node); node.type.visit(this); } } That was when this code lived in Checker, so the patch doesn't make this super obvious. I'll fix the changelog to refer to this defunct helper class by name.
> > > Tools/WebGPUShadingLanguageRI/LateChecker.js:31 > > + constructor() > > + { > > + super(); > > + } > > Nit: you can remove this. Unless you want to be really sure that no > arguments are passed to Visitor.
Fixed.
> > > Tools/WebGPUShadingLanguageRI/LateChecker.js:52 > > + if (!(node.returnType.type instanceof StructType)) > > + throw new WTypeError(node.origin.originString, "Vertex shader " + node.name + " must return a struct."); > > + assertPrimitive(node.returnType); > > Nit: you could probably move this and the one below out of the switch.
Fixed.
Filip Pizlo
Comment 7
2017-09-21 14:36:08 PDT
Landed in
https://trac.webkit.org/changeset/222351/webkit
Radar WebKit Bug Importer
Comment 8
2017-09-27 12:23:25 PDT
<
rdar://problem/34693183
>
Myles C. Maxfield
Comment 9
2018-10-13 19:31:17 PDT
Migrated to
https://github.com/gpuweb/WHLSL/issues/176
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug