[webkit-reviews] review granted: [Bug 177998] Emit SPIR-V from WSL compiler (Part 1) : [Attachment 323116] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 11 15:43:07 PDT 2017


Filip Pizlo <fpizlo at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 177998: Emit SPIR-V from WSL compiler (Part 1)
https://bugs.webkit.org/show_bug.cgi?id=177998

Attachment 323116: Patch

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




--- Comment #6 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 323116
  --> https://bugs.webkit.org/attachment.cgi?id=323116
Patch

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

> Tools/WebGPUShadingLanguageRI/SPIRVCodegen.js:104
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +

So much space!

> Tools/WebGPUShadingLanguageRI/SPIRVCodegen.js:120
> +		       switch (type[0]) {

It seems like you really want the TypeRef, not the instantiated type.  You
should try to see how you can structure this code so that you get that.  Then,
you can case on types more easily.

FWIW the inliner already leaves behind the uninstantiated types in other places
where we already needed them.

I don't think that this concern should block landing, since the plan here is to
land a WIP that we can iterate on.

> Tools/WebGPUShadingLanguageRI/SPIRVCodegen.js:190
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +

So much space!

> Tools/WebGPUShadingLanguageRI/SPIRVCodegen.js:244
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +

So space.

> Tools/WebGPUShadingLanguageRI/SPIRVTypeAnalyzer.js:102
> +    // FIXME: Using toString() in these functions is a hack. Instead, we
should implement a proper type deduper.

Yeah, it's a hack.  But also, can you explain why you need a map at all?

> Tools/WebGPUShadingLanguageRI/SPIRVTypeAnalyzer.js:138
> +	       this.typeMap.set(node.toString(), {id: id, fieldTypes:
fieldTypes});

ES6 lets you say just {id, fieldTypes} in this case.

> Tools/WebGPUShadingLanguageRI/SPIRVVariableAnalyzer.js:141
> +	   let builtInStructs = [
> +	       "struct vec2<> { int32 x; int32 y }",
> +	       "struct vec2<> { uint32 x; uint32 y; }",
> +	       "struct vec2<> { float32 x; float32 y; }",
> +	       "struct vec2<> { float64 x; float64 y; }",
> +	       "struct vec3<> { int32 x; int32 y; int32 z; }",
> +	       "struct vec3<> { uint32 x; uint32 y; uint32 z; }",
> +	       "struct vec3<> { float32 x; float32 y; float32 z; }",
> +	       "struct vec3<> { float64 x; float64 y; float64 z; }",
> +	       "struct vec4<> { int32 x; int32 y; int32 z; int32 w; }",
> +	       "struct vec4<> { uint32 x; uint32 y; uint32 z; uint32 w; }",
> +	       "struct vec4<> { float32 x; float32 y; float32 z; float32 w; }",
> +	       "struct vec4<> { float64 x; float64 y; float64 z; float64 w; }"
> +	   ];
> +	   for (let builtInStruct of builtInStructs) {
> +	       if (node.toString() == builtInStruct) {
> +		   this.result.push({ name: this.nameComponents.join(""), id:
this._currentId++, type: this.typeMap.get(node.toString()).id, location:
this._currentLocation++  });
> +		   return;
> +	       }
> +	   }

I think you really want Intrinsics to stash fields in those types to describe
their spirv behavior.  I think that the vec types should be native types so
that this becomes easier.


More information about the webkit-reviews mailing list