[webkit-reviews] review granted: [Bug 198155] [WHLSL] checkDuplicateFunctions() should not be O(n^2) : [Attachment 371371] patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 198155: [WHLSL] checkDuplicateFunctions() should not be O(n^2)
https://bugs.webkit.org/show_bug.cgi?id=198155

Attachment 371371: patch

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




--- Comment #7 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 371371
  --> https://bugs.webkit.org/attachment.cgi?id=371371
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371371&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:60
> +	   unsigned hash =
IntHash<size_t>::hash(m_function->parameters().size());
> +	   hash ^= m_function->name().hash();

I thought IntegerHasher was better?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:68
> +	   // Note: We can't take into account return type in this hash
function unless
> +	   // we're a cast, since we want two functions with different return
types,
> +	   // but all things else equal, to be "equal" with respect to finding
duplicates.
> +	   // Casts with different return types are always *not* equal to each
other from
> +	   // the perspective of duplicate detection.

I'm not sure this comment provides anything that the next two lines don't
provide.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:93
> +	   // At this point, we have the same name, and the same parameters.
We're only *not* a duplicate now if
> +	   // we're a cast with a different return type. If we're not a cast,
we're now a duplicate. We don't
> +	   // allow overloading of return types. If we're a cast, we're not a
duplicate if our return types 
> +	   // are different.

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:175
> +	   // Native function declarations are never equal to each other. So we
don't need
> +	   // to add them to the set, because they can't collide with each
other. Instead, we
> +	   // just check that no user-defined function is a duplicate.

Can we actually enforce this? If Native functions ever do collide, we should
know about it. Maybe ASSERT() instead of returning false in this case?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLArrayType.h:69
> +	   return WTF::IntHash<unsigned>::hash(m_numElements) ^
m_elementType->hash();

I thought IntegerHasher was better?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLPointerType.h:63
> +	   return this->Base::hash() ^
StringHasher::computeLiteralHash("pointer");

ditto