Bug 199474 – [WHLSL] Remove the phase resolveCallsInFunctions

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

[Robin Morisset]

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.

[Robin Morisset]

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.

[Robin Morisset]

I was confused, this pass is truly not necessary.

[Robin Morisset]

Created attachment 373438 [details]
Patch

[Robin Morisset]

Created attachment 373439 [details]
Patch

[Myles C. Maxfield]

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(...))

[Robin Morisset]

(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.

[WebKit Commit Bot]

Comment on attachment 373439 [details]
Patch

Clearing flags on attachment: 373439

Committed r247127: <https://trac.webkit.org/changeset/247127>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52622712>

[Ryan Haddad]

Reverted r247127 for reason:

Broke the watchOS build.

Committed r247144: <https://trac.webkit.org/changeset/247144>

[Robin Morisset]

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>.

[Robin Morisset]

Created attachment 373519 [details]
Patch

[Robin Morisset]

Created attachment 373520 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 373520 [details]
Patch

Clearing flags on attachment: 373520

Committed r247170: <https://trac.webkit.org/changeset/247170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.