Skip to content

Update Nvptx backend for Zig 0.10 #12878

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

Merged
merged 11 commits into from
Oct 15, 2022
Merged

Update Nvptx backend for Zig 0.10 #12878

merged 11 commits into from
Oct 15, 2022

Conversation

gwenzek
Copy link
Contributor

@gwenzek gwenzek commented Sep 16, 2022

Due to the progress on 0.10 a few things broke Nvptx backend.

Some of the changes I'm pushing here are outside of link/NvPtx.zig, and are modifying shared code.
Let me know if you can think of a better way to structure the changes. From least controversial to more controversial.

  • link/NvPtx.zig -> I'm modifying in place the "Compilation" object. Now I restore the changes before returning. A bit hacky but it allows to reuse the rest of the LLVM backend code without modification there.

  • Target.zig -> PTX only support debug information starting from PTX 7.5. Not sure how to test "ptx >= 7.5".

  • Sema.zig -> allow to use Zig objects in exported .PtxKernel functions. This allow tight integration between CPU/GPU code and was approved by Andrew given that we can change it later if we realize it was a bad idea to expose the ABI like this. Handling of heterogeneous computing #10397 (comment)

  • Module.zig -> sanitize the name of variables. This is required because the names can be like "io.fixed_buffer_stream.FixedBufferStream([]u8).writer" and don't match the C-like identifiers expected by PTX.

Tagging @Snektron who expressed interest in this workstream and @Vexu who kindly merge previous PR (#10189)

Thanks to the progress of self hosted, we can now generate debug information in the PTX files. Also the assembly syntax now works as documented, so it's simpler to use special ptx registers. And I can use the same zig binary for building the device and host code, so it's pretty exciting.

See https://github.com/gwenzek/cudaz/tree/e8895596009c689300fe7c7193fa2dbf7db07629 for user code using this Zig branch.

Comment on lines +710 to +730
if (mod.comp.bin_file.options.target.cpu.arch.isNvptx()) {
for (buffer.items) |*byte| switch (byte.*) {
'{', '}', '*', '[', ']', '(', ')', ',', ' ', '\'' => byte.* = '_',
else => {},
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this not gonna cause linker errors when two symbols map to the same value? Say functions @"A()" and @"A{}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's true. I don't know how robust this should be. I could sanitize and append some hash. Would that be better ?

src/Sema.zig Outdated
Comment on lines 20586 to 20591
.Fn => switch (ty.fnCallingConvention()) {
// For now we want to authorize PTX kernel to use zig objects, even if we end up exposing the ABI.
// The goal is to experiment with more integrated CPU/GPU code.
.PtxKernel => return true,
else => return !Type.fnCallingConventionAllowsZigTypes(ty.fnCallingConvention()),
},
Copy link
Member

Choose a reason for hiding this comment

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

Why not just change fnCallingConventionAllowsZigTypes to return true for .PtxKernel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so fnCallingConventionAllowsZigTypes is already returning true for .PtxKernel. But that's the issue. Because I still want to be able to export the kernels. So we allow exporting functions that don't have Zig types in their CC, and we also allow exporting PtxKernel even if they have Zig types.

@gwenzek
Copy link
Contributor Author

gwenzek commented Sep 20, 2022

Thanks for the first round of review. I'm gonna try to rebase this against @Snektron changes in #12879 and test with LLVM15.

@andrewrk andrewrk merged commit feab1eb into ziglang:master Oct 15, 2022
@andrewrk
Copy link
Member

Nice work.

Would you be willing to write up some corresponding release notes that I can put into the upcoming 0.10.0 release notes?

@gwenzek gwenzek deleted the ptx branch October 15, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants