Bug 200287 – [WHLSL] Make resolveFunction in Checker faster

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Saam Barati]

We spend 40ms(!!!) in it in compute_boids

[Saam Barati]

Created attachment 375211 [details]
WIP

[Saam Barati]

about ~25ms faster. But still not super fast

[Saam Barati]

Gonna check this out

[Saam Barati]

Created attachment 375642 [details]
WIP

rebased

Might not really be faster though

[Saam Barati]

It is actually 6ms faster! Will prepare the patch

[Saam Barati]

Created attachment 375671 [details]
patch

[EWS Watchlist]

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.

[Saam Barati]

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

[Robin Morisset]

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.

[Saam Barati]

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

[Saam Barati]

Created attachment 375675 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 375675 [details]
patch for landing

Clearing flags on attachment: 375675

Committed r248339: <https://trac.webkit.org/changeset/248339>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54017979>