Bug 193080 - [WHLSL] Implement the Type Checker
Summary: [WHLSL] Implement the Type Checker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 193360 193389
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-02 11:08 PST by Myles C. Maxfield
Modified: 2019-01-14 22:02 PST (History)
8 users (show)

See Also:


Attachments
Patch (240.16 KB, patch)
2019-01-02 11:09 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (260.44 KB, patch)
2019-01-02 18:01 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (299.45 KB, patch)
2019-01-03 02:33 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (344.68 KB, patch)
2019-01-03 20:04 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (373.29 KB, patch)
2019-01-04 14:59 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (387.39 KB, patch)
2019-01-04 20:04 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (411.80 KB, patch)
2019-01-07 00:13 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (426.05 KB, patch)
2019-01-08 00:59 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (431.22 KB, patch)
2019-01-08 12:35 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (488.77 KB, patch)
2019-01-08 20:53 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (704.42 KB, patch)
2019-01-09 11:44 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (87.91 KB, patch)
2019-01-11 13:37 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (124.73 KB, patch)
2019-01-12 23:52 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (83.30 KB, patch)
2019-01-13 20:05 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (83.02 KB, patch)
2019-01-13 20:34 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (80.74 KB, patch)
2019-01-14 20:12 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-01-02 11:08:21 PST
Implement the Type Checker
Comment 1 Myles C. Maxfield 2019-01-02 11:09:12 PST
Created attachment 358187 [details]
Patch
Comment 2 Myles C. Maxfield 2019-01-02 18:01:34 PST
Created attachment 358234 [details]
Patch
Comment 3 Myles C. Maxfield 2019-01-03 02:33:33 PST
Created attachment 358247 [details]
Patch
Comment 4 Myles C. Maxfield 2019-01-03 20:04:10 PST
Created attachment 358304 [details]
Patch
Comment 5 Myles C. Maxfield 2019-01-03 22:54:13 PST
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.
Comment 6 Myles C. Maxfield 2019-01-04 14:59:57 PST
Created attachment 358378 [details]
Patch
Comment 7 Myles C. Maxfield 2019-01-04 20:04:28 PST
Created attachment 358425 [details]
Patch
Comment 8 Myles C. Maxfield 2019-01-07 00:13:23 PST
Created attachment 358482 [details]
Patch
Comment 9 Myles C. Maxfield 2019-01-08 00:59:02 PST
Created attachment 358582 [details]
Patch
Comment 10 Myles C. Maxfield 2019-01-08 12:35:30 PST
Created attachment 358621 [details]
Patch
Comment 11 Myles C. Maxfield 2019-01-08 20:53:01 PST
Created attachment 358667 [details]
WIP
Comment 12 Myles C. Maxfield 2019-01-09 11:44:06 PST
Created attachment 358719 [details]
Patch
Comment 13 Myles C. Maxfield 2019-01-11 13:37:20 PST
Created attachment 358936 [details]
WIP
Comment 14 EWS Watchlist 2019-01-11 13:40:15 PST
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.
Comment 15 Myles C. Maxfield 2019-01-12 23:52:33 PST
Created attachment 359002 [details]
Patch
Comment 16 Myles C. Maxfield 2019-01-13 20:05:56 PST
Created attachment 359018 [details]
Patch
Comment 17 EWS Watchlist 2019-01-13 20:10:06 PST
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.
Comment 18 Myles C. Maxfield 2019-01-13 20:34:57 PST
Created attachment 359019 [details]
Patch
Comment 19 Dean Jackson 2019-01-14 10:35:09 PST
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.
Comment 20 Myles C. Maxfield 2019-01-14 10:39:41 PST
(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())
Comment 21 Robin Morisset 2019-01-14 11:30:37 PST
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?
Comment 22 Robin Morisset 2019-01-14 11:35:12 PST
> > 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).
Comment 23 Myles C. Maxfield 2019-01-14 20:03:33 PST
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);
}
Comment 24 Myles C. Maxfield 2019-01-14 20:12:12 PST
Created attachment 359122 [details]
Patch for committing
Comment 25 Myles C. Maxfield 2019-01-14 20:43:19 PST
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
Comment 26 Myles C. Maxfield 2019-01-14 22:01:50 PST
Committed r239975: <https://trac.webkit.org/changeset/239975>
Comment 27 Radar WebKit Bug Importer 2019-01-14 22:02:53 PST
<rdar://problem/47276132>