WSL should have some post-instantiation type checking

[Filip Pizlo]

There are certain things that are easier to check after type instantiation.

Shader type checking is definitely in that category.

[Filip Pizlo]

Created attachment 321439 [details]
the patch

[Filip Pizlo]

Created attachment 321447 [details]
the patch

[Filip Pizlo]

Created attachment 321450 [details]
the patch

[Filip Pizlo]

*** Bug 177098 has been marked as a duplicate of this bug. ***

[Keith Miller]

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]

(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]

Landed in https://trac.webkit.org/changeset/222351/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34693183>

[Myles C. Maxfield]

Migrated to https://github.com/gpuweb/WHLSL/issues/176