[webkit-reviews] review granted: [Bug 200287] [WHLSL] Make resolveFunction in Checker faster : [Attachment 375671] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 6 17:50:30 PDT 2019


Robin Morisset <rmorisset at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 200287: [WHLSL] Make resolveFunction in Checker faster
https://bugs.webkit.org/show_bug.cgi?id=200287

Attachment 375671: patch

https://bugs.webkit.org/attachment.cgi?id=375671&action=review




--- Comment #9 from Robin Morisset <rmorisset at apple.com> ---
Comment on attachment 375671
  --> https://bugs.webkit.org/attachment.cgi?id=375671
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.


More information about the webkit-reviews mailing list