We spend 40ms(!!!) in it in compute_boids
Created attachment 375211 [details] WIP
about ~25ms faster. But still not super fast
Gonna check this out
Created attachment 375642 [details] WIP rebased Might not really be faster though
It is actually 6ms faster! Will prepare the patch
Created attachment 375671 [details] patch
Attachment 375671 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:526: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:599: The parameter name "location" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375671 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=375671&action=review > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:526 > + castReturnType = &downcast<AST::NamedType>(function.type().unifyNode()) ; will fix style
Comment on attachment 375671 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=375671&action=review r=me with comments > Source/WebCore/ChangeLog:15 > + in the hash table such that they still allow constants to resolved to to resolved => to be resolved > Source/WebCore/ChangeLog:24 > + The first two rules are because uint and int constants can be matched against I think only int constants should be coerced to float/uint. If you care enough about the type of a value to make it a uint literal (with the u at the end), doing any kind of silent coercion sounds iffy to me. This only change this changelog, not the code (as int literals can become any of float/int/uint, the three types must be in the same equivalence class anyway). > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:142 > + for (auto& type : m_types) That hash function does not look great to me: it treats foo(int, float*) the same as foo(float*, int). And if two arguments have the same type, then their hashes cancel each others, so foo(int, int) has the same hash as foo(bool, bool). Can you rotate the hash of each argument type by its position in the list of arguments or something like that? > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:146 > + hash ^= WTF::PtrHash<AST::Type*>::hash(&m_castReturnType->unifyNode()); Why is this so different from the hashing of the argument types above? > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:532 > + types.uncheckedAppend(normalizedTypeForFunctionKey(*param->type())); It probably isn't worth changing, but I wonder whether things might be simpler if normalizedTypeForFunctionKey was called inside FunctionKey.hash()/FunctionKey.operator==. It would not only avoid this call, but most of resolveFunction below. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:636 > + HashMap<FunctionKey, Vector<std::reference_wrapper<AST::FunctionDeclaration>, 1>, FunctionKey::Hash, FunctionKey::Traits> m_functions; I'm curious, when do we need to put the hash and traits explicitly and when are they automatically inferred? I've seen that most HashMaps in our codebase don't have them explicit, but I am not sure when they can be omitted.
Comment on attachment 375671 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=375671&action=review >> Source/WebCore/ChangeLog:24 >> + The first two rules are because uint and int constants can be matched against > > I think only int constants should be coerced to float/uint. > If you care enough about the type of a value to make it a uint literal (with the u at the end), doing any kind of silent coercion sounds iffy to me. > This only change this changelog, not the code (as int literals can become any of float/int/uint, the three types must be in the same equivalence class anyway). Good point. Will reword it. (We might currently allow coercing unsigned literals to int literals, but I believe our plan is to prevent that.) >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:142 >> + for (auto& type : m_types) > > That hash function does not look great to me: it treats foo(int, float*) the same as foo(float*, int). And if two arguments have the same type, then their hashes cancel each others, so foo(int, int) has the same hash as foo(bool, bool). > Can you rotate the hash of each argument type by its position in the list of arguments or something like that? Good point. I'll just add the index to each argument's hash. >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:146 >> + hash ^= WTF::PtrHash<AST::Type*>::hash(&m_castReturnType->unifyNode()); > > Why is this so different from the hashing of the argument types above? Because m_castReturnType is a NamedType. A NamedType's identity is based on its unifyNode's pointer value >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:532 >> + types.uncheckedAppend(normalizedTypeForFunctionKey(*param->type())); > > It probably isn't worth changing, but I wonder whether things might be simpler if normalizedTypeForFunctionKey was called inside FunctionKey.hash()/FunctionKey.operator==. > It would not only avoid this call, but most of resolveFunction below. The main motivation for doing this here is avoiding calling it repeatedly during the hash lookup loop (for == and for hash()). I agree it's less elegant, but it should be more performant this way. >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:636 >> + HashMap<FunctionKey, Vector<std::reference_wrapper<AST::FunctionDeclaration>, 1>, FunctionKey::Hash, FunctionKey::Traits> m_functions; > > I'm curious, when do we need to put the hash and traits explicitly and when are they automatically inferred? I've seen that most HashMaps in our codebase don't have them explicit, but I am not sure when they can be omitted. They're never really "automatically" inferred. If you don't specify them, we use the "default" hash and traits in WTF's namespace. And for most things, we've defined defaults. To define a default, you can do something like this: namespace WTF { template<typename T> struct DefaultHash; template<> struct DefaultHash<JSC::B3::Air::Tmp> { typedef JSC::B3::Air::TmpHash Hash; }; template<typename T> struct HashTraits; template<> struct HashTraits<JSC::B3::Air::Tmp> : SimpleClassHashTraits<JSC::B3::Air::Tmp> { }; } // namespace WTF
Created attachment 375675 [details] patch for landing
Comment on attachment 375675 [details] patch for landing Clearing flags on attachment: 375675 Committed r248339: <https://trac.webkit.org/changeset/248339>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54017979>