[webkit-reviews] review granted: [Bug 188402] [WHLSL] Local variables should be statically allocated : [Attachment 350096] 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 188402: [WHLSL] Local variables should be statically allocated
https://bugs.webkit.org/show_bug.cgi?id=188402

Attachment 350096: Patch

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




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

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

Cool patch.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:29
> +    const entryPoints = [];

This is a fairly self-contained block of code. I'd recommend moving it to its
own function.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:30
> +    class OnlyVisitFuncDefsThatAreEntryPoints extends Visitor {

How about "gatherEntryPointDefs" since visitors visit everything?

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:41
> +    const allVariablesAndFunctionParameters = new Set();
> +    const functionsThatAreCalledByEntryPoints = new Set();
> +    class FindAllVariablesAndFunctionParameters extends Visitor {

This is a fairly self-contained block of code. I'd recommend moving it to its
own function.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:53
> +		   node.func.visit(new
FindAllVariablesAndFunctionParameters(node.func));

Doesn't this have exponential runtime because it doesn't dedup functions?

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:59
> +	       node._func = this._currentFunc;

A more descriptive name, please

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:68
> +	       node._func = this._currentFunc;
> +	       if (!this._currentFunc.isEntryPoint)
> +		   allVariablesAndFunctionParameters.add(node);

Why does visitVariableDecl() have a super call but visitFuncParameter not?

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:83
> +    program.add(ptrToGlobalStructType);

This doesn't seem right, because the parser will never put a PtrType at the
global level, so we probably shouldn't do that either.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:100
> +    let counter = 0;
> +    const varToFieldMap = new Map();
> +    for (let varOrParam of allVariablesAndFunctionParameters) {
> +	   const fieldName =
`field${counter++}_${varOrParam._func.name}_${varOrParam.name}`;
> +	   globalStructType.add(new Field(varOrParam.origin, fieldName,
varOrParam.type));
> +	   varToFieldMap.set(varOrParam, fieldName);
> +    }
> +
> +    for (let func of functionsThatAreCalledByEntryPoints) {
> +	   if (func.returnType.name !== "void") {
> +	       const fieldName = `field${counter++}_return_${func.name}`;
> +	       globalStructType.add(new Field(func.origin, fieldName,
func.returnType));
> +	       func.returnFieldName = fieldName;
> +	   }
> +    }

This is a fairly self-contained block of code. I'd recommend moving it to its
own function.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:139
> +	       get func()
> +	       {
> +		   return this._func;
> +	       }

Not sure if this is necessary if all callers are local to the class.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:157
> +		   const possibleAndOverloads =
program.globalNameContext.get(Func, functionName);
> +		   const callExpressionResolution =
CallExpression.resolve(node.origin, possibleAndOverloads, functionName, [
this.globalStructVariableRef ], [ ptrToGlobalStructTypeRef ]);

It's kind of unfortunate we have to reverse-engineer what would have happened
earlier in the compiler. Can we run this stage earlier so we don't have to do
this?

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:171
> +		       return super.visitVariableRef(node);

Isn't this an error?

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:180
> +		       return new Assignment(node.origin,
this._dereferencedCallExpressionForFieldName(node, node.type,
varToFieldMap.get(node)), node.initializer.visit(this), node.type);

Nodes need to get assigned (zero-filled) even if they don't have an
initializer. We should add a test for this.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:183
> +		   else if (node == this.variableDecl)
> +		       return node;

I'd move this first

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:199
> +			   const anonymousVariable = new
AnonymousVariable(node.origin, type);

What is the purpose of the anonymous variables? Why not assign directly into
the global struct?

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:208
> +			  
exprs.push(this._dereferencedCallExpressionForFieldName(node.func,
node.func.returnType, node.func.returnFieldName));

Neat.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:210
> +		       node.argumentList = [ this.globalStructVariableRef ];

Are you sure it's wise for them all to be using the exact same VariableRef?
Seems like we should store a creation lambda instead of the raw variable
itself.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:220
> +		   if (node.value && this._func.returnFieldName)

If these don't match, seems like this should be an error.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:223
> +		   return new CommaExpression(node.origin, [
> +		       new Assignment(node.origin,
this._dereferencedCallExpressionForFieldName(this._func, this._func.returnType,
this._func.returnFieldName), node.value, this._func.returnType),
> +		       new Return(node.origin) ]);

Indentation

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:240
> +	       if (node._newParameters)

This pollutes the FuncDef nodes. I'd prefer a side-table.

> Tools/WebGPUShadingLanguageRI/AllocateAtEntryPoints.js:250
> +	       if (node.func.returnFieldName)
> +		   node._returnType = node.resultType =
TypeRef.wrap(program.types.get("void"));

Cool.

> Tools/WebGPUShadingLanguageRI/EBufferBuilder.js:-33
> -    constructor(program)
> -    {
> -	   super();
> -	   this._program = program;
> -    }
> -    

What? What's the point of this class if you can never construct it? I don't see
any other constructors or static functions.

> Tools/WebGPUShadingLanguageRI/Func.js:57
> +    set parameters(newValue)

Not a great variable name.

> Tools/WebGPUShadingLanguageRI/SynthesizeStructAccessors.js:51
> +	   function createFieldType()
> +	   {
> +	       return field.type.visit(new Rewriter());
> +	   }
> +
> +	   function createTypeRef()
> +	   {
> +	       return TypeRef.wrap(type);
> +	   }

Do we need these?

> Tools/WebGPUShadingLanguageRI/SynthesizeStructAccessors.js:101
>	       nativeFunc = new NativeFunc(
> -		   field.origin, "operator." + field.name + "=",
createTypeRef(),
> +		   field.origin, "operator&." + field.name, new
PtrType(field.origin, addressSpace, createFieldType()),
>		   [
> -		       new FuncParameter(field.origin, null, createTypeRef()),
> -		       new FuncParameter(field.origin, null, createFieldType())
> +		       new FuncParameter(
> +			   field.origin, null,
> +			   new PtrType(field.origin, addressSpace,
createTypeRef()))
>		   ],
>		   isCast, shaderType);
> -	       setupImplementationData(nativeFunc, ([base, value], offset,
structSize, fieldSize) => {
> -		   let result = new EPtr(new EBuffer(structSize), 0);
> -		   result.copyFrom(base, structSize);
> -		   result.plus(offset).copyFrom(value, fieldSize);
> -		   return result;
> +	       setupImplementationData(nativeFunc, ([base], offset, structSize,
fieldSize) => {
> +		   base = base.loadValue();
> +		   if (!base)
> +		       throw new WTrapError(field.origin.originString, "Null
dereference");
> +		   return EPtr.box(base.plus(offset));
>	       });
>	       program.add(nativeFunc);

diff really made a mess of things, didn't it