Bug 193080 – [WHLSL] Implement the Type Checker

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

[Myles C. Maxfield]

Implement the Type Checker

[Myles C. Maxfield]

Created attachment 358187 [details]
Patch

[Myles C. Maxfield]

Created attachment 358234 [details]
Patch

[Myles C. Maxfield]

Created attachment 358247 [details]
Patch

[Myles C. Maxfield]

Created attachment 358304 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 358304 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLInferTypes.cpp:116
> +    // Pull out the two preferredTypes
> +    // Make sure both of the resolvableTypes canResolve() with both of the preferredTypes
> +    // Pick the winner
> +    // Resolve both of the resolvable types
> +    // return the winner

The subclasses need to be the same. If the two subclasses are Int/Uint/FloatLiteralType, commit them to be Int/Uint/Float.
Currently, two NullLiteralTypes don't match.

[Myles C. Maxfield]

Created attachment 358378 [details]
Patch

[Myles C. Maxfield]

Created attachment 358425 [details]
Patch

[Myles C. Maxfield]

Created attachment 358482 [details]
Patch

[Myles C. Maxfield]

Created attachment 358582 [details]
Patch

[Myles C. Maxfield]

Created attachment 358621 [details]
Patch

[Myles C. Maxfield]

Created attachment 358667 [details]
WIP

[Myles C. Maxfield]

Created attachment 358719 [details]
Patch

[Myles C. Maxfield]

Created attachment 358936 [details]
WIP

[EWS Watchlist]

Attachment 358936 [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 7 files


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

[Myles C. Maxfield]

Created attachment 359002 [details]
Patch

[Myles C. Maxfield]

Created attachment 359018 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:9:  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 7 files


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

[Myles C. Maxfield]

Created attachment 359019 [details]
Patch

[Dean Jackson]

Comment on attachment 359019 [details]
Patch

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

rs=me

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:166
> +    if (callExpression.name() == "operator&[]" && types.size() == 2) {

Ditto a previous comment about using raw strings.

[Myles C. Maxfield]

(In reply to Dean Jackson from comment #19)
> Comment on attachment 359019 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359019&action=review
> 
> rs=me
> 
> > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:166
> > +    if (callExpression.name() == "operator&[]" && types.size() == 2) {
> 
> Ditto a previous comment about using raw strings.

We have an overload of operator== that takes a string literal. This is faster than turning the literal into a String object. (Also the literal is cool because the string length is generated at compile time, so the implementation doesn’t have to call strlen())

[Robin Morisset]

Comment on attachment 359019 [details]
Patch

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

I've only reviewed the beginning so far (first 400 lines). The rest of the review will come later.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:120
> +static Optional<AST::NativeFunctionDeclaration> resolveWithOperatorAnderIndexer(AST::CallExpression& callExpression, AST::ArrayReferenceType& firstArgument, const Intrinsics& intrinsics)

I don't see in which case the Optional<> result is ever not present?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:122
> +    bool isRestricted = false;

These two booleans could be const I think.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:131
> +static Optional<AST::NativeFunctionDeclaration> resolveWithOperatorLength(AST::CallExpression& callExpression, AST::UnnamedType& firstArgument, const Intrinsics& intrinsics)

Same as above.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:133
> +    bool isRestricted = false;

Same as above.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:297
> +            if (is<AST::PointerType>(unnamedType))

Is it on purpose that PointerTypes are rejected here, but not array reference types nor array types?
In the spec I have all three forbidden in that position.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:300
> +        return true;

If the kind is CheckKind::Index, the spec also requires a check that the second argument is one of uchar, ushort, uint, char, short or int.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:310
> +            if (is<AST::PointerType>(unnamedType))

Same question as above.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:332
> +    };

I have not carefully checked this bunch of checks to find the matching getter, they are a bit hard to read. We'll just have to remember to test them.

If the kind is CheckKind::Index, the spec also requires a check that the second argument is one of uchar, ushort, uint, char, short or int.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:351
> +            return is<AST::PointerType>(unnamedType) || is<AST::ArrayReferenceType>(unnamedType);

This seems weird to me: why would we require that the first parameter of an ander be a pointer or reference? Don't we want to forbid it instead?

[Robin Morisset]

> > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:351
> > +            return is<AST::PointerType>(unnamedType) || is<AST::ArrayReferenceType>(unnamedType);
> 
> This seems weird to me: why would we require that the first parameter of an
> ander be a pointer or reference? Don't we want to forbid it instead?

Forget this, I got confused, your code is correct (and under takes a pointer as their first argument).

[Myles C. Maxfield]

Comment on attachment 359019 [details]
Patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:297
>> +            if (is<AST::PointerType>(unnamedType))
> 
> Is it on purpose that PointerTypes are rejected here, but not array reference types nor array types?
> In the spec I have all three forbidden in that position.

I was following this: https://github.com/gpuweb/WHLSL/blob/master/Source/Checker.mjs#L309 "if (func.parameterTypes[0].unifyNode.isPtr)" I think you're right that this is better.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:300
>> +        return true;
> 
> If the kind is CheckKind::Index, the spec also requires a check that the second argument is one of uchar, ushort, uint, char, short or int.

I think it's also totally reasonable to have a second argument be an enum type. I'll make the code match the spec, and we can discuss adding enums to the spec too.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:351
>> +            return is<AST::PointerType>(unnamedType) || is<AST::ArrayReferenceType>(unnamedType);
> 
> This seems weird to me: why would we require that the first parameter of an ander be a pointer or reference? Don't we want to forbid it instead?

Anders increment pointers. The following is a correct ander:

struct Foo {
    int x;
    int y;
}
thread int* operator&.second(thread Foo* foo) {
    return &(foo->y);
}

[Myles C. Maxfield]

Created attachment 359122 [details]
Patch for committing

[Myles C. Maxfield]

Comment on attachment 359019 [details]
Patch

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

>>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:297
>>> +            if (is<AST::PointerType>(unnamedType))
>> 
>> Is it on purpose that PointerTypes are rejected here, but not array reference types nor array types?
>> In the spec I have all three forbidden in that position.
> 
> I was following this: https://github.com/gpuweb/WHLSL/blob/master/Source/Checker.mjs#L309 "if (func.parameterTypes[0].unifyNode.isPtr)" I think you're right that this is better.

https://github.com/gpuweb/WHLSL/commit/9817625e5ed2685e94f1b976d719a8684c5d4e40

>>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:300
>>> +        return true;
>> 
>> If the kind is CheckKind::Index, the spec also requires a check that the second argument is one of uchar, ushort, uint, char, short or int.
> 
> I think it's also totally reasonable to have a second argument be an enum type. I'll make the code match the spec, and we can discuss adding enums to the spec too.

https://github.com/gpuweb/WHLSL/commit/9817625e5ed2685e94f1b976d719a8684c5d4e40

[Myles C. Maxfield]

Committed r239975: <https://trac.webkit.org/changeset/239975>

[Radar WebKit Bug Importer]

<rdar://problem/47276132>