Bug 187988 – [WHLSL] Remove generics from the interpreter

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Thomas Denney]

Generics are no longer in the specification; completely removing them greatly simplifies the code base.

[Thomas Denney]

Created attachment 345819 [details]
Patch

[Thomas Denney]

Created attachment 346235 [details]
WIP Patch

[Thomas Denney]

Created attachment 346920 [details]
WIP

[Thomas Denney]

Created attachment 347022 [details]
Patch

[EWS Watchlist]

Attachment 347022 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:194:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thomas Denney]

Created attachment 347023 [details]
Patch

[EWS Watchlist]

Comment on attachment 347023 [details]
Patch

Attachment 347023 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8856445

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 347091 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Thomas Denney]

Created attachment 347226 [details]
Patch

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Comment on attachment 347226 [details]
Patch

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

Initial comments; I haven't read many of the large new code blocks yet.

> Tools/ChangeLog:128
> +        * WebGPUShadingLanguageRI/OperatorArrayRefLength.js: Added, previously

You can just say "ditto"

> Tools/WebGPUShadingLanguageRI/All.js:89
>  load("FloatLiteralType.js");

Why not FuncInstantiator? We shouldn't need to instantiate functions anymore.

> Tools/WebGPUShadingLanguageRI/All.js:120
>  load("NativeFuncInstance.js");

Why NativeTypeInstance but not NativeFuncInstance? We shouldn't need to instantiate functions.

> Tools/WebGPUShadingLanguageRI/CallExpression.js:42
>      get isCast() { return this._isCast; }

Why not remove the typeArguments field? Calls should never have type arguments.

> Tools/WebGPUShadingLanguageRI/CallExpression.js:64
> +            const func = this._resolveByInstantiation(program);

Are we super sure this is the right place to synthesize these functions? If the user specifies their own operator&[], do we want that to be an error or do we want it to override the synthesized one? Is there ever a case when function overload resolution might prefer the synthesized one even if a user-created one exists?

Rather than implementing this first, and then figuring out how it fits into the existing language definition, we should first decide how it should work, then implement.

> Tools/WebGPUShadingLanguageRI/Checker.js:675
> +        let result = node.resolve(node.possibleOverloads, this._programt);

programt

> Tools/WebGPUShadingLanguageRI/FuncInstantiator.js:27
>  class FuncInstantiator {

Why do we need to instantiate functions?

> Tools/WebGPUShadingLanguageRI/InferTypesForCall.js:42
> +        if (func.returnType.toString() == "vector") {
> +            returnType.unify(unificationContext, func.returnType)
> +        }

Same as above.

> Tools/WebGPUShadingLanguageRI/Inline.js:60
> +    let func = program.funcInstantiator.getUnique(overload.func);

Why is this necessary?

> Tools/WebGPUShadingLanguageRI/Inliner.js:38
>          if (result.nativeFuncInstance)

Since we shouldn't need to monomorphize native functions, there's no point in having nativeFuncInstance any more.

> Tools/WebGPUShadingLanguageRI/Inliner.js:41
> +            let func = this._program.funcInstantiator.getUnique(result.func);

Ditto.

> Tools/WebGPUShadingLanguageRI/NameResolver.js:57
> +        let contextWithParameters = new NameContext(contextWithReturnType);

Why do we need separate contexts here?

> Tools/WebGPUShadingLanguageRI/NameResolver.js:60
> +            parameter.visit(checkerWithReturnType);
>              contextWithParameters.add(parameter);

These don't match each other. Are you sure this is right?

> Tools/WebGPUShadingLanguageRI/NameResolver.js:126
> +        for (let arg of node.typeArguments)
> +            arg.visit(this);

Same as above. TypeRefs shouldn't have type arguments.

> Tools/WebGPUShadingLanguageRI/NativeFunc.js:33
>          this._implementationData = null;

We'll never instantiate native functions, so instantiateImplementation is unnecessary.

> Tools/WebGPUShadingLanguageRI/NativeFuncInstance.js:27
>  class NativeFuncInstance extends Func {

Shouldn't be necessary, since we'll never instantiate native functions.

> Tools/WebGPUShadingLanguageRI/NativeParameterizedType.js:27
> +class NativeParameterizedType extends NativeType {

Why not just give NativeType a possibly-empty array of type arguments? When would you ever have a NativeType that isn't a NativeParameterizedType?

> Tools/WebGPUShadingLanguageRI/Parse.js:220
> -                return new TypeOrVariableRef(result, result.text);
> +                return new TypeRef(result, result.text, []);

Cool.

> Tools/WebGPUShadingLanguageRI/Parse.js:242
> +    function parseType(allowTypeArguments = true)

You never pass false to this function. Either pass false somewhere or remove the argument.

> Tools/WebGPUShadingLanguageRI/Parse.js:252
>          let type = new TypeRef(name, name.text, typeArguments);

TypeRefs should never accept type arguments.

> Tools/WebGPUShadingLanguageRI/Parse.js:892
> +            if (args.length)
> +                return NativeParameterizedType.fromNameAndTypeArguments(origin, name.text, args);
> +            else
> +                return new NativeType(origin, name.text);

Like above, don't understand why we need a new type

> Tools/WebGPUShadingLanguageRI/Rewriter.js:301
> +        // FIXME Check if the type arguments are ever actually mapped over

? CallExpressions shouldn't have type arguments

> Tools/WebGPUShadingLanguageRI/StructLayoutBuilder.js:77
>          Node.visit(node.nativeFuncInstance, this);

No need for nativeFuncInstance

> Tools/WebGPUShadingLanguageRI/SwizzleOp.js:109
>  \ No newline at end of file

:(

> Tools/WebGPUShadingLanguageRI/SynthesizeStructAccessors.js:36
>                  nativeFunc.instantiateImplementation = substitution => {

We shouldn't ever be instantiating native functions.

> Tools/WebGPUShadingLanguageRI/SynthesizeStructAccessors.js:75
> +                return field.type.visit(new AutoWrapper());

Substitution extends Rewriter, not AutoWrapper. Why did you choose AutoWrapper?

> Tools/WebGPUShadingLanguageRI/Test.html:183
> +<style>
> +    pre {
> +        margin:0;
> +    }
> +</style>

?

> Tools/WebGPUShadingLanguageRI/TypeRef.js:49
> -        let result = new TypeRef(type.origin, type.name, typeArguments);
> +        let result = new TypeRef(type.origin, type.name, []);

Why not remove the field entirely? TypeRefs should never have arguments; the arguments belong to the type itself.

> Tools/WebGPUShadingLanguageRI/TypeRef.js:101
>      {

Why didn't you change unifyImpl?

> Tools/WebGPUShadingLanguageRI/UnificationContext.js:-42
> -        if (!a.isUnifiable) {

We shouldn't remove the concept of unifiable. Literals and null are unifiable, but other types aren't.

> Tools/WebGPUShadingLanguageRI/UnificationContext.js:100
>          for (let typeArgument of this.typeArguments()) {

This loop body should never be executed.

> Tools/WebGPUShadingLanguageRI/Visitor.js:75
> +    visitTypeRef(node) {}

New lines, please

> Tools/WebGPUShadingLanguageRI/WSL.md:42
>  On top of this bare C-like foundation, WSL adds secure versions of familiar C++ features:
>  
>  - Type-safe pointers (`^`) and array references (`[]`).
> -- Generics to replace templates.
>  - Operator overloading. Built-in operations like `int operator+(int, int)` are just native functions.
>  - Cast overloading. Built-in casts like `operator int(double)` are just native functions.
>  - Getters and setters. Property accesses like `vec.x` resolve to overloads like `float operator.field(float4)`.
>  - Array access overloading. Array accesses like `vec[0]` resolve to verloads like `float operator[](float4, uint)`.
>  
> -In the following sections, WSL is shown by example starting with its C-like foundation and then building up to include more sophisticated features like generics.
> +In the following sections, WSL is shown by example starting with its C-like foundation and then building up to include more sophisticated features.
>  
>  ## Common subset of C and WSL

We should just delete this file. The language has changed a lot and a better description will be in the W3C.

[Myles C. Maxfield]

Comment on attachment 347226 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/BuiltinVectorCasts.js:27
> +class BuiltinVectorCasts {

These are constructors, not casts.

> Tools/WebGPUShadingLanguageRI/BuiltinVectorCasts.js:41
> +        let sum = 0;
> +        for (let paramSize of this.parameterSizes)
> +            sum += paramSize;
> +        return sum;

There's no Javascript idiomatic way of doing this?

> Tools/WebGPUShadingLanguageRI/BuiltinVectorCasts.js:80
> +                for (let j = 0; j < this.parameterSizes[i]; j++)

Seems brittle to not use StructLayoutBuilder.

> Tools/WebGPUShadingLanguageRI/BuiltinVectorEqualityOperator.js:27
> +class BuiltinVectorEqualityOperator {

There aren't that many of these, and they don't need to be generated at runtime. It's valuable to implement as much as possible in the language. Why implement them natively?

> Tools/WebGPUShadingLanguageRI/BuiltinVectorGetter.js:66
> +            return EPtr.box(vec.get(this.index));

Ditto about StructLayoutBuilder

> Tools/WebGPUShadingLanguageRI/BuiltinVectorIndexGetter.js:63
> +        func.implementation = ([vec, index], node) => {
> +            const indexValue = index.loadValue();
> +            if (indexValue >= 0 && indexValue < this.size)
> +                return EPtr.box(vec.get(index.loadValue()));
> +            else
> +                throw new Error(`${indexValue} out of bounds for vector of size ${this.size}`);
> +        };

Ditto about implementing in the language

> Tools/WebGPUShadingLanguageRI/BuiltinVectorIndexSetter.js:69
> +        func.implementation = ([base, index, value], node) => {
> +            const indexValue = index.loadValue();
> +            if (indexValue >= 0 && indexValue < this.size) {
> +                let result = new EPtr(new EBuffer(this.size), 0);
> +                result.copyFrom(base, this.size);
> +                result.plus(indexValue).copyFrom(value, 1);
> +                return result;
> +            } else
> +                throw new Error(`${indexValue} out of bounds for vector of size ${this.size}`);
> +        };
> +        func.implementationData = this;

Ditto about implementing in the language

> Tools/WebGPUShadingLanguageRI/CallExpression.js:105
> +    _resolveToOperatorAnderIndexer(program)

"to" isn't quite the right preposition. Maybe "with?"

> Tools/WebGPUShadingLanguageRI/CallExpression.js:107
> +        let arrayRefType = this.argumentTypes[0];

Are there any other places where the user can implement a function with the exact same signature as something in the standard library and it won't be a compile error?

It seems like we can create these things in Checker (like we are doing now) but they should be created unconditionally, as if they happened to be in the standard library the whole time. That way, if/when we add generics back in, there won't be a problem.

> Tools/WebGPUShadingLanguageRI/CallExpression.js:108
> +        if (!arrayRefType.isArrayRef)

What about arrays proper?

> Tools/WebGPUShadingLanguageRI/CallExpression.js:141
> +            func.implementation = (args, node) => EPtr.box(arrayType.numElementsValue);

All the other ones have an associated class to do the instantiation, but not this one? Seems to me that the main value of OperatorArrayRefLength is toString() for debugging, so why shouldn't this block have the same benefit?

> Tools/WebGPUShadingLanguageRI/CallExpressionTypeArgumentResolver.js:37
> +        // Call expressions with type arguments are only ever operator casts.

We should check this with code.

> Tools/WebGPUShadingLanguageRI/Checker.js:258
> +            if (!node.typeArguments[i] instanceof Type) {

When would this happen?

> Tools/WebGPUShadingLanguageRI/Checker.js:289
> +        if (!node.numElements.type.equalsWithCommit(this._program.intrinsics.uint))
> +            throw new WTypeError(node.origin.originString, "Vector size must be a uint");

Vector types have to be length 2, 3, or 4

> Tools/WebGPUShadingLanguageRI/FlattenedStructOffsetGatherer.js:72
> +                offset: this._offset + i,

Can't we at least check the size of the inner type (even though it will always be 1 for now)?

> Tools/WebGPUShadingLanguageRI/Intrinsics.js:510
> +        for (let swizzle of SwizzleOp.functions())
> +            this._map.set(swizzle.toString(), func => swizzle.instantiateImplementation(func));

Once the parser is super fast, is it really necessary to implement these natively?

> Tools/WebGPUShadingLanguageRI/Intrinsics.js:519
> +        for (let cast of BuiltinVectorCasts.functions())
> +            this._map.set(cast.toString(), func => cast.instantiateImplementation(func));

Ditto.

> Tools/WebGPUShadingLanguageRI/OperatorBool.js:48
> +            const typeNames = [ "uchar", "uint", "int", "float" ].concat(allVectorTypeNames());

Does HLSL actually have these? If so, we should implement these in the standard library, not natively.

> Tools/WebGPUShadingLanguageRI/StandardLibrary.js:217
> +const VectorElementTypes = [ /*"bool",*/ "uchar", /*"char", "ushort", "short",*/ "uint", "int", /* "half", */"float" ];
> +const VectorElementSizes = [ 2, 3, 4 ];

These don't seem to belong in the standard library. They're not text, and they're properties of the language itself.

> Tools/WebGPUShadingLanguageRI/StandardLibrary.js:230
> +standardLibrary += OperatorAnderIndexer.functions().join(";\n") + ";\n";

I don't understand. If we're generating these dynamically inside Checker, why are we listing them in the standard library?

> Tools/WebGPUShadingLanguageRI/SynthesizeArrayOperatorLength.js:62
> +        let nativeFunc = new NativeFunc(
> +            arrayType.origin, "operator.length", uint,
> +            [ new FuncParameter(arrayType.origin, null, paramType) ],
> +            false, null);
> +        nativeFunc.implementation = ([array]) => EPtr.box(arrayType.numElementsValue);

What's the difference between this and _resolveToOperatorLength()? Can we get away with one but not the other?

> Tools/WebGPUShadingLanguageRI/SynthesizeCopyConstructorOperator.js:50
> +    program.visit(new FindAllTypes());

Same thing - we don't know all the types until we run Checker (because of things like MakeArrayRef operators) but Checker needs these functions to be present. So I'm not sure having a prepass is helpful.

> Tools/WebGPUShadingLanguageRI/SynthesizeDefaultConstructorOperator.js:50
> +    program.visit(new FindAllTypes());

Ditto.

> Tools/WebGPUShadingLanguageRI/SynthesizeOperatorBool.js:29
> +    for (let type of program.types.values()) {

Same comment as the other synthesize functions

> Tools/WebGPUShadingLanguageRI/VectorType.js:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

2018. Generally we should be updating the year any time any file is touched.

> Tools/WebGPUShadingLanguageRI/VectorType.js:32
> +            throw new Error(`Vector type takes 2 arguments, ${typeArguments.length} given.`);

Why is this Error and not WError?

[Thomas Denney]

Created attachment 347343 [details]
Patch

[Thomas Denney]

Created attachment 347344 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 347344 [details]
Patch

Clearing flags on attachment: 347344

Committed r235096: <https://trac.webkit.org/changeset/235096>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dawei Fenton (:realdawei)]

(In reply to WebKit Commit Bot from comment #16)
> Comment on attachment 347344 [details]
> Patch
> 
> Clearing flags on attachment: 347344
> 
> Committed r235096: <https://trac.webkit.org/changeset/235096>

Looks like webkit is failing to compile on Sierra Debug after this revision:

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20%28Build%29/builds/12787

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20%28Build%29/builds/12787/steps/compile-webkit/logs/stdio

[Dawei Fenton (:realdawei)]

(In reply to David Fenton (:realdawei) from comment #18)
> (In reply to WebKit Commit Bot from comment #16)
> > Comment on attachment 347344 [details]
> > Patch
> > 
> > Clearing flags on attachment: 347344
> > 
> > Committed r235096: <https://trac.webkit.org/changeset/235096>
> 
> Looks like webkit is failing to compile on Sierra Debug after this revision:
> 
> https://build.webkit.org/builders/Apple%20Sierra%20Debug%20%28Build%29/
> builds/12787
> 
> https://build.webkit.org/builders/Apple%20Sierra%20Debug%20%28Build%29/
> builds/12787/steps/compile-webkit/logs/stdio

Actually it cleared up, disregard.

[Radar WebKit Bug Importer]

<rdar://problem/43534358>

[Myles C. Maxfield]

Comment on attachment 347344 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/InferTypesForCall.js:42
> +        if (func.returnType.toString() == "vector") {
> +            returnType.unify(unificationContext, func.returnType)
> +        }

This seems wrong.

[Myles C. Maxfield]

Comment on attachment 347344 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/UnificationContext.js:-105
> -            let preparation = node.prepareToVerify(this);

This is how literals get their preferred type. We may wish to use this infrastructure to match the "4" in vector<float, 4> with the "int" type.

Also, this is the only call of prepareToVerify(), so if we remove this line, we should remove the implementations too.

[Myles C. Maxfield]

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

[Thomas Denney]

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