Bug 199474 - [WHLSL] Remove the phase resolveCallsInFunctions
Summary: [WHLSL] Remove the phase resolveCallsInFunctions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-03 15:52 PDT by Robin Morisset
Modified: 2019-07-05 13:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch (19.00 KB, patch)
2019-07-03 16:57 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (18.84 KB, patch)
2019-07-03 16:59 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (20.71 KB, patch)
2019-07-05 12:00 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (20.71 KB, patch)
2019-07-05 12:02 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2019-07-03 15:52:16 PDT
This pass only stores into each property access and call expression vectors of all the functions it might be calling, for use by the Checker afterwards.
But the checker is perfectly able to compute a pointer to these vectors by itself.
So by removing this pass, we gain the following:

- One less pass over the AST
- No need to copy these vectors (which can be large for heavily overloaded functions, of which there are quite a few in the stdlib)
- No need to have these vectors in the expressions, saving 24 bytes per CallExpression and 72 bytes per PropertyAccessExpression
- No need to allocate and then destroy these vectors.
Comment 1 Robin Morisset 2019-07-03 16:31:34 PDT
Turns out this pass is also used for some other miscellaneous name resolving, so it cannot be removed altogether. But the vector copying and storing at least should be avoidable.
Comment 2 Robin Morisset 2019-07-03 16:55:34 PDT
I was confused, this pass is truly not necessary.
Comment 3 Robin Morisset 2019-07-03 16:57:17 PDT
Created attachment 373438 [details]
Patch
Comment 4 Robin Morisset 2019-07-03 16:59:03 PDT
Created attachment 373439 [details]
Patch
Comment 5 Myles C. Maxfield 2019-07-03 17:14:10 PDT
Comment on attachment 373439 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1028
> +        getterFunction = resolveFunction(m_program, getterFunctions, getterArgumentTypes, getterName, propertyAccessExpression.origin(), m_intrinsics);

nit: if ((getterFunction = resolveFunction(...))
Comment 6 Robin Morisset 2019-07-03 17:18:08 PDT
(In reply to Myles C. Maxfield from comment #5)
> Comment on attachment 373439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373439&action=review
> 
> > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1028
> > +        getterFunction = resolveFunction(m_program, getterFunctions, getterArgumentTypes, getterName, propertyAccessExpression.origin(), m_intrinsics);
> 
> nit: if ((getterFunction = resolveFunction(...))

I tried that at first, but it no longer fit on one line on my screen, and was not really a readability improvement.
Comment 7 WebKit Commit Bot 2019-07-03 18:15:44 PDT
Comment on attachment 373439 [details]
Patch

Clearing flags on attachment: 373439

Committed r247127: <https://trac.webkit.org/changeset/247127>
Comment 8 WebKit Commit Bot 2019-07-03 18:15:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-07-03 18:18:21 PDT
<rdar://problem/52622712>
Comment 10 Ryan Haddad 2019-07-04 11:53:44 PDT
Reverted r247127 for reason:

Broke the watchOS build.

Committed r247144: <https://trac.webkit.org/changeset/247144>
Comment 11 Robin Morisset 2019-07-05 11:50:38 PDT
Found the problem: we replace in-place a PropertyAccessExpression by a CallExpression.
Since I've shrunk PropertyAccessExpression, this is no longer safe on 32-bit platforms.

The good news is that there is no bare PropertyAccessExpression, all of them are either DotExpressions or IndexExpressions, both of which have an extra field that make them large enough to replace by a CallExpression in-place.
So I just have to add a downcast to the right subclass before the call to replaceWith<CallExpression>.
Comment 12 Robin Morisset 2019-07-05 12:00:24 PDT
Created attachment 373519 [details]
Patch
Comment 13 Robin Morisset 2019-07-05 12:02:11 PDT
Created attachment 373520 [details]
Patch
Comment 14 WebKit Commit Bot 2019-07-05 13:09:45 PDT
Comment on attachment 373520 [details]
Patch

Clearing flags on attachment: 373520

Committed r247170: <https://trac.webkit.org/changeset/247170>
Comment 15 WebKit Commit Bot 2019-07-05 13:09:47 PDT
All reviewed patches have been landed.  Closing bug.