Bug 200522 - [WHLSL] Devirtualize the AST
Summary: [WHLSL] Devirtualize the AST
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-07 16:45 PDT by Saam Barati
Modified: 2019-08-09 18:03 PDT (History)
14 users (show)

See Also:


Attachments
WIP (81.64 KB, patch)
2019-08-07 19:54 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (123.43 KB, patch)
2019-08-08 01:51 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (115.56 KB, patch)
2019-08-08 16:30 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (133.55 KB, patch)
2019-08-08 19:55 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (162.63 KB, patch)
2019-08-09 10:14 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (161.75 KB, patch)
2019-08-09 10:19 PDT, Saam Barati
ews: commit-queue-
Details | Formatted Diff | Diff
patch (167.32 KB, patch)
2019-08-09 14:08 PDT, Saam Barati
rmorisset: review+
Details | Formatted Diff | Diff
patch for landing (166.43 KB, patch)
2019-08-09 14:45 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2019-08-07 16:45:01 PDT
...
Comment 1 Saam Barati 2019-08-07 19:54:16 PDT
Created attachment 375782 [details]
WIP
Comment 2 Saam Barati 2019-08-08 01:51:26 PDT
Created attachment 375791 [details]
WIP
Comment 3 Saam Barati 2019-08-08 14:03:04 PDT
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.
Comment 4 Robin Morisset 2019-08-08 14:33:52 PDT
(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
Comment 5 Saam Barati 2019-08-08 16:30:53 PDT
Created attachment 375856 [details]
WIP
Comment 6 Saam Barati 2019-08-08 19:55:30 PDT
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.
Comment 7 Saam Barati 2019-08-09 10:14:21 PDT
Created attachment 375929 [details]
patch
Comment 8 Saam Barati 2019-08-09 10:16:03 PDT
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.
Comment 9 Build Bot 2019-08-09 10:16:17 PDT
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.
Comment 10 Saam Barati 2019-08-09 10:19:02 PDT
Created attachment 375930 [details]
patch
Comment 11 Build Bot 2019-08-09 10:21:23 PDT
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.
Comment 12 Saam Barati 2019-08-09 10:22:50 PDT
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().
Comment 13 Robin Morisset 2019-08-09 11:40:54 PDT
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.
Comment 14 Build Bot 2019-08-09 12:22:21 PDT
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
Comment 15 Sam Weinig 2019-08-09 13:46:59 PDT
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.
Comment 16 Saam Barati 2019-08-09 14:08:13 PDT
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()
Comment 17 Saam Barati 2019-08-09 14:08:46 PDT
Created attachment 375951 [details]
patch
Comment 18 Build Bot 2019-08-09 14:12:57 PDT
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.
Comment 19 Robin Morisset 2019-08-09 14:15:44 PDT
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!
Comment 20 Saam Barati 2019-08-09 14:32:31 PDT
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.
Comment 21 Saam Barati 2019-08-09 14:45:53 PDT
Created attachment 375963 [details]
patch for landing
Comment 22 Build Bot 2019-08-09 14:49:00 PDT
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.
Comment 23 WebKit Commit Bot 2019-08-09 16:30:36 PDT
Comment on attachment 375963 [details]
patch for landing

Clearing flags on attachment: 375963

Committed r248488: <https://trac.webkit.org/changeset/248488>
Comment 24 WebKit Commit Bot 2019-08-09 16:30:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2019-08-09 16:31:18 PDT
<rdar://problem/54147553>
Comment 26 Saam Barati 2019-08-09 18:03:05 PDT
build fix in:
https://trac.webkit.org/changeset/248493/webkit