[webkit-reviews] review granted: [Bug 189615] [WHLSL] Implement trap statements in Metal code generation : [Attachment 350438] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Thomas Denney
<tdenney at apple.com>'s request for review:
Bug 189615: [WHLSL] Implement trap statements in Metal code generation
https://bugs.webkit.org/show_bug.cgi?id=189615

Attachment 350438: Patch

https://bugs.webkit.org/attachment.cgi?id=350438&action=review




--- Comment #12 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 350438
  --> https://bugs.webkit.org/attachment.cgi?id=350438
Patch

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

We can use another bug about making the metal codegen do packing/unpacking at
the beginning/end of every entry point.

> Tools/WebGPUShadingLanguageRI/Metal/MSLInsertTrapParameter.js:54
> +    const functionVisitedOrder = [];
> +    const entryPointFunctions = new Set();
> +    class FindEntryPoints extends Visitor {
> +	   visitFuncDef(node)
> +	   {
> +	       if (node.isEntryPoint && !entryPointFunctions.has(node)) {
> +		   entryPointFunctions.add(node);
> +		   functionVisitedOrder.push(node);
> +	       }
> +	   }
> +    }
> +    program.visit(new FindEntryPoints());
> +
> +    const functionsCalledByEntryPoints = new Set();
> +    class FindFunctionsCalledByEntryPoints extends Visitor {
> +	   visitCallExpression(node)
> +	   {
> +	       super.visitCallExpression(node);
> +	       if (node.func instanceof FuncDef &&
!functionsCalledByEntryPoints.has(node.func)) {
> +		   functionsCalledByEntryPoints.add(node.func);
> +		   functionVisitedOrder.push(node.func);
> +		   node.func.visit(this);
> +	       }
> +	   }
> +    }
> +    for (let entryPoint of entryPointFunctions)
> +	   entryPoint.visit(new FindFunctionsCalledByEntryPoints());

We've had to duplicate this code a bunch of times. We should save the results,
perhaps on the program object itself.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:48
> +    _emitTrap(setTrapValue)

I would split this function into two separate pieces and update the callers to
call the relevant piece

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:54
> +	   if (this._func.returnType.name !== "void") {

Should compare against program.intrinsics.void

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:58
> +	       this._add(`return ${id};`);

Every return point needs to do unpacking/packing to whatever form Metal
expects. This appears to do no packing.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:60
> +	       this._add("return;");

Easier to understand if the comparison was an equality comparison, and you
switch the order of the two blocks

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:268
> +	   this._add(`}`);
> +	   return `(*(${ptr}))`;

What? The line above is "return". When will this ever be executed?

> Tools/WebGPUShadingLanguageRI/Test.js:2503
> +    const numVars = 50; // Originally this was 1024 but that was too slow

No reason for the comment, 50 is fine.

> Tools/WebGPUShadingLanguageRI/Test.js:8323
> +tests.INTERPRETER_DISABLED_trapTransitively = () => {

We should modify the interpreter to have the same behavior as the real code
gen. Should be easy - just make the root entry point catch the TrapException