| Summary: | [WHLSL] Add native function synthesis passes | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | commit-queue, dino, ews-watchlist, fpizlo, jonlee, justin_fan, rmorisset, simon.fraser, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 192826 | ||||||||||
| Bug Blocks: | 193080 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Myles C. Maxfield
2019-01-11 13:05:05 PST
Created attachment 358932 [details]
Patch
Attachment 358932 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:52: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeEnumerationFunctions.cpp:43: This { should be at the end of the previous line [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeEnumerationFunctions.cpp:55: This { should be at the end of the previous line [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeStructureAccessors.cpp:47: This { should be at the end of the previous line [whitespace/braces] [4]
Total errors found: 4 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Style failures are false negatives. Comment on attachment 358932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358932&action=review > Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:48 > + std::sort(functions.begin(), functions.end(), [](const AST::FunctionDeclaration& a, const AST::FunctionDeclaration& b) -> bool { Do we need to actually sort all functions by alphabetic order, or just to sort them in any order to find duplicates? If the later, I think I would switch the order of the check, sorting by length first: if (a.name().length() < b.name.length()) return true; if (a.name().length() > b.name.length()) return false; for (unsigned i = 0; i < a.name().length(); ++i) { if (a.name()[i] < b.name()[i]) return true; if (a.name()[i] > b.name()[i]) return false; } return false; It is probably premature optimization on my part though, feel free to ignore it. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:65 > + if (functions[i].get().isCast()) { I think this case and the next could be merged and simplified if (functions[i].get().isCast() && !matches(functions[i].get().type(), functions[j].get().type())) continue; bool same = true; for (... if (same) return false > Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:92 > + if (functions[i].get().name() == "operator&[]" && functions[i].get().parameters().size() == 2 I'm not super fond of these special cases for native functions. In particular, it seems easy to forget one, if we add more native functions. For example, are we currently correctly dealing with getters and setters to structure fields? I don't really have a suggestion for a cleaner approach though. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLRecursiveTypeChecker.cpp:45 > + if (addResult.isNewEntry) { Do we really want to error and return early when adding a new entry? I would have expected us to do it when there is already an entry, so when !addResult.isNewEntry. Same comment about the other versions of visit in this file. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLRecursiveTypeChecker.cpp:64 > + checkErrorAndVisit(structureDefinition); I may have misunderstood the interface to your visitor, but I would have expected doing the recursion on each field of the struct, and not on the definition itself. Same comment about the other versions of visit in this file. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeArrayOperatorLength.cpp:44 > + void visit(AST::ArrayType& arrayType) override Does this implementation recurse throughout the entire AST, just to find some declarations? It seems wasteful to for example go into deep expressions. Is there any case where it is actually required? Comment on attachment 358932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358932&action=review > Source/WebCore/ChangeLog:21 > + - CheckDuplicateFunctions which makes sure the same function isn't defined twice > + - Intrinsics, which remembers all of the native types so they can be referred to by the > + rest of the compiler > + - RecursiveTypeChecker which makes sure types don't refer to themselves > + - SynthesizeArrayOperatorLength which creates operator.length() functions for arrays > + - SynthesizeConstructors which creates copy constructors and default constructors for all > + types > + - SynthesizeEnumerationFunctions which provides cast operators between enum types and their > + base types > + - SynthesizeStructureAccessors which provides getters, setters, and anders for each member > + of a struct While it would be annoying, I wish these were in separate patches. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:61 > + : m_textureTypeNames { String("Texture1D", String::ConstructFromLiteral), > + String("RWTexture1D", String::ConstructFromLiteral), > + String("Texture1DArray", String::ConstructFromLiteral), > + String("RWTexture1DArray", String::ConstructFromLiteral), > + String("Texture2D", String::ConstructFromLiteral), > + String("RWTexture2D", String::ConstructFromLiteral), > + String("Texture2DArray", String::ConstructFromLiteral), > + String("RWTexture2DArray", String::ConstructFromLiteral), > + String("Texture3D", String::ConstructFromLiteral), > + String("RWTexture3D", String::ConstructFromLiteral), > + String("TextureCube", String::ConstructFromLiteral) } > + , m_textureInnerTypeNames { String("uchar", String::ConstructFromLiteral), > + String("ushort", String::ConstructFromLiteral), > + String("uint", String::ConstructFromLiteral), > + String("char", String::ConstructFromLiteral), > + String("short", String::ConstructFromLiteral), > + String("int", String::ConstructFromLiteral), > + String("half", String::ConstructFromLiteral), > + String("float", String::ConstructFromLiteral) } > + , m_depthTextureInnerTypes { String("half", String::ConstructFromLiteral), > + String("float", String::ConstructFromLiteral) } You can use "foo"_s for all these. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:78 > + if (nativeTypeDeclaration.name() == "void") Maybe we should have static consts for these strings, rather than using the raw text? > Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.h:135 > + Vector<String> m_textureTypeNames; > + Vector<String> m_textureInnerTypeNames; Do these ever change? Can they be static? > Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.h:139 > + Vector<String> m_depthTextureInnerTypes; Ditto. Comment on attachment 358932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358932&action=review >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:48 >> + std::sort(functions.begin(), functions.end(), [](const AST::FunctionDeclaration& a, const AST::FunctionDeclaration& b) -> bool { > > Do we need to actually sort all functions by alphabetic order, or just to sort them in any order to find duplicates? > If the later, I think I would switch the order of the check, sorting by length first: > if (a.name().length() < b.name.length()) > return true; > if (a.name().length() > b.name.length()) > return false; > for (unsigned i = 0; i < a.name().length(); ++i) { > if (a.name()[i] < b.name()[i]) > return true; > if (a.name()[i] > b.name()[i]) > return false; > } > return false; > > It is probably premature optimization on my part though, feel free to ignore it. I can see why this would be faster if many of the strings shared prefixes, but it would be slower if all the strings were the same length and placed evenly along the search space. I suppose we have a lot of operators, which would share a prefix, so presumably this would be a performance optimization. It's probably premature, but it's pretty simple so I'll just do it. >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:92 >> + if (functions[i].get().name() == "operator&[]" && functions[i].get().parameters().size() == 2 > > I'm not super fond of these special cases for native functions. > In particular, it seems easy to forget one, if we add more native functions. > For example, are we currently correctly dealing with getters and setters to structure fields? > I don't really have a suggestion for a cleaner approach though. Yeah, these match _resolveByInstantiation() inside CallExpression. The idea is that if a function is dynamically generated at runtime, it shouldn't be overridable by the user. This gives it the appearance that those functions were present all along (though the error message will be different). Perhaps moving both this code and the _resolveByInstantiation() code into the same file would help? What do you think? Created attachment 358984 [details]
WIP
Attachment 358984 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeEnumerationFunctions.cpp:43: This { should be at the end of the previous line [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeEnumerationFunctions.cpp:55: This { should be at the end of the previous line [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeStructureAccessors.cpp:47: This { should be at the end of the previous line [whitespace/braces] [4]
Total errors found: 4 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 358932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358932&action=review >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:78 >> + if (nativeTypeDeclaration.name() == "void") > > Maybe we should have static consts for these strings, rather than using the raw text? Aha! I tried this when writing it, but it didn't work because I didn't know about https://stackoverflow.com/questions/4891067/weird-undefined-symbols-of-static-constants-inside-a-struct-class. Now that I did that, it works! Comment on attachment 358932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358932&action=review >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLRecursiveTypeChecker.cpp:45 >> + if (addResult.isNewEntry) { > > Do we really want to error and return early when adding a new entry? > I would have expected us to do it when there is already an entry, so when !addResult.isNewEntry. > Same comment about the other versions of visit in this file. Argh, this must have gotten messed up during the various iterations of this patch. Good catch. >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLRecursiveTypeChecker.cpp:64 >> + checkErrorAndVisit(structureDefinition); > > I may have misunderstood the interface to your visitor, but I would have expected doing the recursion on each field of the struct, and not on the definition itself. > Same comment about the other versions of visit in this file. Right, this got messed up when I was refactoring Visitor. I really need to finish enough of this compiler so I can start running the test suite.... >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeArrayOperatorLength.cpp:44 >> + void visit(AST::ArrayType& arrayType) override > > Does this implementation recurse throughout the entire AST, just to find some declarations? > It seems wasteful to for example go into deep expressions. Is there any case where it is actually required? I think it does need to for casts, since a cast can hold an ArrayType and can be arbitrarily deep within an expression. Created attachment 358994 [details]
Patch for committing
Attachment 358994 [details] did not pass style-queue:
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeEnumerationFunctions.cpp:43: This { should be at the end of the previous line [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeEnumerationFunctions.cpp:55: This { should be at the end of the previous line [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeStructureAccessors.cpp:47: This { should be at the end of the previous line [whitespace/braces] [4]
Total errors found: 3 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 358932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358932&action=review >>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:48 >>> + std::sort(functions.begin(), functions.end(), [](const AST::FunctionDeclaration& a, const AST::FunctionDeclaration& b) -> bool { >> >> Do we need to actually sort all functions by alphabetic order, or just to sort them in any order to find duplicates? >> If the later, I think I would switch the order of the check, sorting by length first: >> if (a.name().length() < b.name.length()) >> return true; >> if (a.name().length() > b.name.length()) >> return false; >> for (unsigned i = 0; i < a.name().length(); ++i) { >> if (a.name()[i] < b.name()[i]) >> return true; >> if (a.name()[i] > b.name()[i]) >> return false; >> } >> return false; >> >> It is probably premature optimization on my part though, feel free to ignore it. > > I can see why this would be faster if many of the strings shared prefixes, but it would be slower if all the strings were the same length and placed evenly along the search space. I suppose we have a lot of operators, which would share a prefix, so presumably this would be a performance optimization. It's probably premature, but it's pretty simple so I'll just do it. That being said, most of the operators have names that are the same length... Comment on attachment 358994 [details] Patch for committing Clearing flags on attachment: 358994 Committed r239902: <https://trac.webkit.org/changeset/239902> |