Skip to content

Conversation

@qjia7
Copy link
Contributor

@qjia7 qjia7 commented Nov 29, 2019

This change is to reduce the overhead of shaderKey comparison.
Previously, we used a very long string 'userCode' as part
of the shader key which is time consuming when search the shader key.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

This change is to reduce the overhead of shaderKey comparison.
Previously, we used a very long string 'userCode' as part
of the shader key which is time consuming when search the shader key.
@qjia7 qjia7 changed the title [WIP] webgpu: Add shaderKey as the program member webgpu: Add shaderKey as the program member Dec 2, 2019
@qjia7
Copy link
Contributor Author

qjia7 commented Dec 2, 2019

@annxingyuan Please have a preview if it's in the right direction. It seems that latest webgpu backend has some regressions. Lots of tests will fail because of SPIRV validation error. I will look at that first.

Copy link
Contributor

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qjia7 This is great! However I would like this to be opt-in for each webgpu program. Can you make sure shaderKey is an optional property and that we default to the user code when shaderKey is undefined?

Also please keep me posted re the SPIRV validation errors - thanks so much for looking into those!

@qjia7
Copy link
Contributor Author

qjia7 commented Dec 3, 2019

Can you make sure shaderKey is an optional property and that we default to the user code when shaderKey is undefined?

Please see if the latest change is what you want. I am wondering maybe in future we can change userCode to a function getUserCode(). In this case, we can move a lot logic to getUserCode and don't need to do them every time when we call the program constructor.

please keep me posted re the SPIRV validation errors

Filed a bug to SPIRV-Tools. See KhronosGroup/SPIRV-Tools#3085

Copy link
Contributor

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qjia7 Thank you this is great!

@annxingyuan annxingyuan merged commit 65d60ae into tensorflow:master Dec 3, 2019
axinging added a commit to axinging/tfjs that referenced this pull request Jan 14, 2020
FIX

hat happens:
Run case one by one (yarn test --grep=), pass. But when run several together. Some case may fail.

Example case:
Case may be failed due to cache reasons(div/Arithmetic_test.ts):
'gradient: Tensor1D with int32',               // Actual != expected.
'gradient: Tensor2D',                          // Actual != expected.
'gradient: Tensor2D / Tensor2D w/ broadcast',  // Actual != expected.

Root cause:
This problem is introduced by two reasons:
1), makeShaderKey called before the real shader generated(compileProgram and makeShader).
Which means, two different shaders may possibly share the same shader key.

2), The shader key optimization(tensorflow#2451), which use
several key properties of the shader instead of the full shader source as shader key.
For most case, especially all properties are self-contained in the shader`s WebGPUProgram
file, this works. But when some properties are originated from the shader_preprocessor.ts,
two different shader may be collided with the same shader key.

Currently the fix is to disable the shader key and shader cache first.
The follow up todo is: tensorflow#2669.

Also add square and neg operators.
@qjia7 qjia7 deleted the shaderKey branch August 13, 2020 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants