Bug 200522 – [WHLSL] Devirtualize the AST

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

[Saam Barati]

...

[Saam Barati]

Created attachment 375782 [details]
WIP

[Saam Barati]

Created attachment 375791 [details]
WIP

[Saam Barati]

Robin had a nice solution on how to solve it:
1. Use a deleter that does dispatch on type.
2. Make various UniqueRef use this custom deleter
3. Make RefCounted accept a custom deleter so Ref<T> Just Works

I think this removes the ugliness that I defined here.

[Robin Morisset]

(In reply to Saam Barati from comment #3)
> Robin had a nice solution on how to solve it:
> 1. Use a deleter that does dispatch on type.
> 2. Make various UniqueRef use this custom deleter
> 3. Make RefCounted accept a custom deleter so Ref<T> Just Works
> 
> I think this removes the ugliness that I defined here.

A couple points I'd like to clarify here:
- The custom deleter must downcast the base pointer to the right derived pointer, so then call the default delete.
- we can make the destructor of the base classes protected, making sure we never use the default deleter

[Saam Barati]

Created attachment 375856 [details]
WIP

[Saam Barati]

Created attachment 375883 [details]
WIP

Some of these templates are painful. I don't think I know how to combine SFINAE with a specialization of the default_delete struct. So I'm just defining it explicitly for each class in the hierarchy, which is less than stellar.

[Saam Barati]

Created attachment 375929 [details]
patch

[Saam Barati]

Comment on attachment 375929 [details]
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLDefaultDelete.h:39
> +#define DEFINE_DEFAULT_DELETE(type) \
> +    namespace std { \
> +        template<> \
> +        struct default_delete<WebCore::WHLSL::AST::type> { \
> +            void operator()(WebCore::WHLSL::AST::type* value) \
> +            { \
> +                WebCore::WHLSL::AST::type::destroy(*value); \
> +            } \
> +        }; \
> +    } \

I wanted to use SFINAE to define these for all subtypes in a particular class hierarchy. However, I couldn't figure out how to do this. I ran into a bunch of issues, basically:
https://stackoverflow.com/questions/12858839/using-sfinae-for-template-class-specialisation

and this doesn't work (as it seems it only worked because it was a compiler bug)
https://stackoverflow.com/questions/31213516/stdhash-specialization-using-sfinae/31213703#31213703

So I just succumbed to splatting this out manually for each class.

[EWS Watchlist]

Attachment 375929 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/WorkerScriptLoader.h:82:  std::default_delete is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 76 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 375930 [details]
patch

[EWS Watchlist]

Attachment 375930 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/WorkerScriptLoader.h:82:  std::default_delete is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 375930 [details]
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLDefaultDelete.h:39
> +#define DEFINE_DEFAULT_DELETE(type) \
> +    namespace std { \
> +        template<> \
> +        struct default_delete<WebCore::WHLSL::AST::type> { \
> +            void operator()(WebCore::WHLSL::AST::type* value) \
> +            { \
> +                WebCore::WHLSL::AST::type::destroy(*value); \
> +            } \
> +        }; \
> +    } \

I think I can further optimize this at compile time. Basically, if is_final<type>, just call delete. Else, call destroy().

[Robin Morisset]

Comment on attachment 375930 [details]
patch

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

> Source/WTF/wtf/UniqueRef.h:43
> +    using Deleter = std::default_delete<T>;

Since std::default_delete<T> is already the default deleter for std::unique_ptr, what is the point to add it explicitly?

> Source/WebCore/ChangeLog:15
> +        that when they're used inside UniqueRef, unique_ptr, Ref, RefPtr, that

Extra "that" at the end.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:160
> +            std::unique_ptr<AST::Expression> makePointerExpression(new AST::MakePointerExpression(functionDefinition.codeLocation(), WTFMove(structVariableReference), AST::AddressEscapeMode::DoesNotEscape));

What is the reason for not using std::make_unique anymore here?

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLDefaultDelete.h:39
>> +    } \
> 
> I think I can further optimize this at compile time. Basically, if is_final<type>, just call delete. Else, call destroy().

I agree, it would be nice.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.cpp:107
> +    if (is<DotExpression>(*this))

Is there a reason to use is<> instead of switching on the kind?
How does is<> even work without a vtable?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:148
> +    CodeLocation codeLocation() const { return CodeLocation(m_codeLocation, m_codeLocation); }

Why are you removing half the information in CodeLocation ?!
Just having the start of the expression is not enough for good error messages.
Consider "(x + y) * z". If we want to emit an error message for the '+' or for the '*' operation they will have the same starting offset, but not the same end offset.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:153
> +    unsigned m_codeLocation;

Once m_codeLocation is again a CodeLocation, there won't be space here for m_kind.
Please put it after m_typeAnnotation, there are 5 padding bytes there.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLNamedType.h:50
> +        , m_codeLocation(location.startOffset())

Same as for Expressions, please keep m_codeLocation a CodeLocation unless you have a good reason.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLPropertyAccessExpression.h:-50
> -    virtual ~PropertyAccessExpression() = default;

This is not a concrete class, so please make the destructor protected.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLStatement.h:65
> +        : m_codeLocation(codeLocation.startOffset())

Same as for Expression and NamedType: why lose half the information in codeLocation?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLType.h:121
> +

extra newline.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLUnnamedType.h:50
> +        , m_codeLocation(location.startOffset())

Same comment as for the other classes.

[EWS Watchlist]

Comment on attachment 375930 [details]
patch

Attachment 375930 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12885916

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases

[Sam Weinig]

Comment on attachment 375930 [details]
patch

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

> Source/WTF/wtf/UniqueRef.h:77
> +    static_assert(sizeof(m_ref) == sizeof(uintptr_t), "Don't use a stateful deleter.");

This message could be clearer as to why.

> Source/WebCore/ChangeLog:27
> +        This is a 3ms speedup on compute_boids, which is about a 10% improvement
> +        in the WHLSL compiler.

Nice. Out of curiosity, how much did this effect metal codegen time?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:153
> +    unsigned m_codeLocation;

Why is storing only the startOffset ok for these?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLNamedType.h:48
> +    NamedType(Kind kind, CodeLocation location, String&& name)

For Statement and Expression, the Kind came last. I think it would be good to maintain consistency.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLResolvableType.h:50
> +    ResolvableType(Kind kind)
> +        : Type(kind)
> +    { }

This can probably be protected as well. Should also be marked explicit.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLType.h:67
> +    Type(Kind kind)
> +        : m_kind(kind)
> +    { }

Should be explicit.

[Saam Barati]

Comment on attachment 375930 [details]
patch

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

>> Source/WTF/wtf/UniqueRef.h:43
>> +    using Deleter = std::default_delete<T>;
> 
> Since std::default_delete<T> is already the default deleter for std::unique_ptr, what is the point to add it explicitly?

If I remember correctly, it's for the constructor below. I can confirm in a few by using godbolt.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:160
>> +            std::unique_ptr<AST::Expression> makePointerExpression(new AST::MakePointerExpression(functionDefinition.codeLocation(), WTFMove(structVariableReference), AST::AddressEscapeMode::DoesNotEscape));
> 
> What is the reason for not using std::make_unique anymore here?

because there were compiler errors when casting up to the base class.

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.cpp:107
>> +    if (is<DotExpression>(*this))
> 
> Is there a reason to use is<> instead of switching on the kind?
> How does is<> even work without a vtable?

is<> is defined to be whatever we want it to be. In this case, in our implementation, we just call a function.

I use is<> just because it's convenient here. is<>'s implementation will branch on kind()

[Saam Barati]

Created attachment 375951 [details]
patch

[EWS Watchlist]

Attachment 375951 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/WorkerScriptLoader.h:82:  std::default_delete is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 76 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robin Morisset]

Comment on attachment 375951 [details]
patch

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

r=me

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReplaceWith.h:41
> +    Old::destruct(old);

Nice catch!

[Saam Barati]

Comment on attachment 375951 [details]
patch

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

> Source/WTF/wtf/UniqueRef.h:47
> +        : m_ref(other.m_ref.release())

this is the only line needed. This nicely allows us to cast downwards.

[Saam Barati]

Created attachment 375963 [details]
patch for landing

[EWS Watchlist]

Attachment 375963 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/WorkerScriptLoader.h:82:  std::default_delete is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 76 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 375963 [details]
patch for landing

Clearing flags on attachment: 375963

Committed r248488: <https://trac.webkit.org/changeset/248488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54147553>

[Saam Barati]

build fix in:
https://trac.webkit.org/changeset/248493/webkit