Bug 201185 – [WHLSL] Inline typedef'd types during Metal code generation to simplify generated code while also making it easier to read

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Sam Weinig]

[WHLSL] Inline typedef'd types during Metal code generation to simplify generated code while also making it easier to read

[Sam Weinig]

Created attachment 377373 [details]
Patch

[Sam Weinig]

Created attachment 377420 [details]
before.txt

[Sam Weinig]

Created attachment 377421 [details]
after.txt

[Sam Weinig]

With this change, the generated metal for compute_boids goes from 913 lines to 751 lines and becomes quite a bit more straightforward to read.

[Saam Barati]

Comment on attachment 377373 [details]
Patch

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

This is very nice. Does it have an effect on performance? If so, maybe we should cache the resulting strings?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:-320
> -    auto iterator = m_unnamedTypeMapping.find(UnnamedTypeKey { unnamedType });

Maybe it’s worth keeping this so we don’t repeatedly make the same strings and just mapping it to MangledOrNativeTypeName now?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:276
> +        auto& parent = typeReference.resolvedType();

This is a weird name for this variable. I’m not sure it needs to exist. I’d just omit giving this expression a variable

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:281
> +        auto& parent = pointerType.elementType();

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:287
> +        auto& parent = arrayType.type();

Ditto

[Sam Weinig]

(In reply to Saam Barati from comment #5)
> Comment on attachment 377373 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377373&action=review
> 
> This is very nice. Does it have an effect on performance?
On it's own, its about a 20% speedup in metal code generation time.

> If so, maybe we should cache the resulting strings?

I added the caching and it looks like it is another 5% speedup, so I kept it.

> 
> > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:-320
> > -    auto iterator = m_unnamedTypeMapping.find(UnnamedTypeKey { unnamedType });
> 
> Maybe it’s worth keeping this so we don’t repeatedly make the same strings
> and just mapping it to MangledOrNativeTypeName now?

I added a separate cache and lazily cached the generated string for AST::ArrayType and AST::Pointer. In a followup, we should also look at caching the NativeTypeDeclaration names.

> 
> > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:276
> > +        auto& parent = typeReference.resolvedType();
> 
> This is a weird name for this variable. I’m not sure it needs to exist. I’d
> just omit giving this expression a variable

Yeah, these made more sense in their previous context. Fixed.

[Sam Weinig]

Committed r249209: <https://trac.webkit.org/changeset/249209>

[Radar WebKit Bug Importer]

<rdar://problem/54799097>

[Darin Adler]

Comment on attachment 377373 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:314
> +        auto iterator = m_namedTypeMapping.find(&namedType);
> +        ASSERT(iterator != m_namedTypeMapping.end());
> +        return iterator->value;

Not sure if others agree but I prefer to write cases like this differently, with double hash table lookup in debug builds:

    ASSERT(m_namedTypeMapping.contains(&namedType));
    return m_namedTypeMapping.get(&namedType);

It also seems like HashMap could offer a variation of get that asserts containership. This idiom seems to come up a lot.

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/249209/webkit

broke 19 webgpu/ tests on Mojave wk2 

build:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/6225

results:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249209%20(6225)/results.html

Does this look like an easy fix? otherwise we will need to roll it out.

[Sam Weinig]

(In reply to Darin Adler from comment #9)
> Comment on attachment 377373 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377373&action=review
> 
> > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:314
> > +        auto iterator = m_namedTypeMapping.find(&namedType);
> > +        ASSERT(iterator != m_namedTypeMapping.end());
> > +        return iterator->value;
> 
> Not sure if others agree but I prefer to write cases like this differently,
> with double hash table lookup in debug builds:
> 
>     ASSERT(m_namedTypeMapping.contains(&namedType));
>     return m_namedTypeMapping.get(&namedType);
> 
> It also seems like HashMap could offer a variation of get that asserts
> containership. This idiom seems to come up a lot.

I also prefer this idiom. Not sure why I didn't change it as I was moving code around. Do you have thoughts on a potential name for the new HashMap function. I can't think of anything great. My best thoughts are renaming the existing get() to something like tryGet()/peek()/maybeGet() and then use get() to be the asserting get().

[Sam Weinig]

(In reply to Truitt Savell from comment #10)
> It looks like the changes in https://trac.webkit.org/changeset/249209/webkit
> 
> broke 19 webgpu/ tests on Mojave wk2 
> 
> build:
> https://build.webkit.org/builders/
> Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/6225
> 
> results:
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r249209%20(6225)/results.html
> 
> Does this look like an easy fix? otherwise we will need to roll it out.

Please roll it out.

[Truitt Savell]

Reverted r249209 for reason:

Broke 19 webgpu/ tests

Committed r249215: <https://trac.webkit.org/changeset/249215>

[Sam Weinig]

(In reply to Truitt Savell from comment #13)
> Reverted r249209 for reason:
> 
> Broke 19 webgpu/ tests
> 
> Committed r249215: <https://trac.webkit.org/changeset/249215>

Thanks.

[Darin Adler]

Comment on attachment 377373 [details]
Patch

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

>>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:314
>>> +        return iterator->value;
>> 
>> Not sure if others agree but I prefer to write cases like this differently, with double hash table lookup in debug builds:
>> 
>>     ASSERT(m_namedTypeMapping.contains(&namedType));
>>     return m_namedTypeMapping.get(&namedType);
>> 
>> It also seems like HashMap could offer a variation of get that asserts containership. This idiom seems to come up a lot.
> 
> I also prefer this idiom. Not sure why I didn't change it as I was moving code around. Do you have thoughts on a potential name for the new HashMap function. I can't think of anything great. My best thoughts are renaming the existing get() to something like tryGet()/peek()/maybeGet() and then use get() to be the asserting get().

I like the tryGet/get combo, but I’m also wonder what state of the art is for this is in collection class design. Does this exist in Swift collections or is just this unpacking an optional there? Does anyone else do this particularly well? One problem is that if we were making tryGet today we’d probably use an optional instead of an empty value, which of course would be less efficient and elegant for pointers and pointer-like things. There’s even a possibility that we might want three of these: one that uses optional and two that use the empty value, one that asserts and one that doesn’t. Or even four with one that does a runtime check and aborts. I do worry about overcomplicating things, but I would love to have a convenient get that asserts success for day to day programming. Might even want versions of the get/find family that guard the empty and deleted values so the callers don’t need to do that. It’s a performance optimization to require callers to check this, but makes the API harder to use.

[Myles C. Maxfield]

WHLSL is no longer relevant.