Implement the Type Checker
Created attachment 358187 [details] Patch
Created attachment 358234 [details] Patch
Created attachment 358247 [details] Patch
Created attachment 358304 [details] Patch
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.
Created attachment 358378 [details] Patch
Created attachment 358425 [details] Patch
Created attachment 358482 [details] Patch
Created attachment 358582 [details] Patch
Created attachment 358621 [details] Patch
Created attachment 358667 [details] WIP
Created attachment 358719 [details] Patch
Created attachment 358936 [details] WIP
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.
Created attachment 359002 [details] Patch
Created attachment 359018 [details] Patch
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.
Created attachment 359019 [details] Patch
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.
(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 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?
> > 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 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); }
Created attachment 359122 [details] Patch for committing
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
Committed r239975: <https://trac.webkit.org/changeset/239975>
<rdar://problem/47276132>