Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TVM] Fixed SPIR-V codegen incorrectly not declaring the interface for the entry point #1400

Merged
merged 6 commits into from
Jul 9, 2018
Merged

[TVM] Fixed SPIR-V codegen incorrectly not declaring the interface for the entry point #1400

merged 6 commits into from
Jul 9, 2018

Conversation

alex-weaver
Copy link
Contributor

The SPIR-V codegen was not generating code compliant with the spec, which was manifesting in all of the vulkan unit tests failing on Windows 10 with a GTX 960 GPU. Specifically, whenever a vulkan kernel was invoked, it would only ever succeed in updating the first element of the input tensor.

This turned out to be due to the codegen not generating the interface list of OpEntryPoint. For the code that TVM generates, the interface list should contain both the workgroup id and the local invocation id. See https://www.khronos.org/registry/spir-v/specs/1.0/SPIRV.html#OpEntryPoint for details.

SPIRV-Tools has recently been updated to detect this situation (see KhronosGroup/SPIRV-Tools#1589), so one way to verify that TVM is generating correct code is to take the output of get_source, use spirv-as to generate a binary SPIR-V file, and then run a recent release of spirv-val on the binary file.

The fix in this PR does solve the problem, but I'm not sure if it's the best way of addressing this, so I'm definitely open to suggestions on how this could be done better (99% of this PR was tracking down the bug in the first place!)

@tqchen
Copy link
Member

tqchen commented Jul 8, 2018

Thanks for the fix! Here is a possibly better way to do so:

  • DeclareKenrelFunction -> CommitKernelFunction(func_id, name) (call after codegen)
    • Add all the interfaces get used
  • In the beginning, func_id = builder->NewFunction() which returns a new function value

@tqchen tqchen self-requested a review July 8, 2018 18:41
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

see comments

@tqchen tqchen added status: need update need update based on feedbacks status: review in progress labels Jul 8, 2018
}

void IRBuilder::CommitKernelFunction(const Value& func_id, const std::string& name) {
//if (uses_workgroup_id) {
Copy link
Member

Choose a reason for hiding this comment

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

please remove comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, noticed those just after committing. They are removed from the latest commit

@tqchen tqchen merged commit 561e548 into apache:master Jul 9, 2018
@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks status: review in progress labels Jul 9, 2018
@alex-weaver alex-weaver deleted the spirv-entrypoint branch July 10, 2018 16:47
tqchen pushed a commit to tqchen/tvm that referenced this pull request Aug 4, 2018
sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants