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

🔗 Expose the Dxc linker #71

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

🔗 Expose the Dxc linker #71

wants to merge 3 commits into from

Conversation

Jasper-Bekkers
Copy link
Member

@Jasper-Bekkers Jasper-Bekkers commented Oct 11, 2023

Alright so this needs a bit of work still, but I'm happy to be where we are.

  1. DXC and now this library allow for linking DXIL
  2. I've released spirv-linker to link spirv binaries

Unfortunately, dxc doesn't compile use-export.hlsl properly when compiled for spirv. The compiler doesn't output any sensible ops for the shader, and as a result, actually linking anything for spirv can't quite be done at the moment.

Relevant DXC bug: microsoft/DirectXShaderCompiler#5854

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Neat stuff, but seeing this really makes me crave for tightening up the Result handling in these functions (in turn making them much more useful to end users) and other improvements :)

Comment on lines +36 to +37
let exports = exports.ok().unwrap().get_result().unwrap();
let use_exports = use_exports.ok().unwrap().get_result().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Ugh it really is time for me to PR that error handling simplification, this Result<DxcOperationResult, (DxcOperationResult, HRESULT)> is so useless 😬

I think you even need to call get_status() etc :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, but I didn't feel like cleaning it up in this PR. There's a bit of an ambiguity there too because there are multiple possible failure operations making this quite complex. (e.g. link and compile can fail without an actual compiler or linker error)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah definitely not. I was more so thinking of the other way around: any of these might return an error code (including .compile() or .link()), but DxcOperationResult might still contain an error blob so that there's more context why the HRESULT was false. It just doesn't make sense that there's also get_status() giving you two more HRESULTs on top of the function return.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can probably replace all of this with a thiserror type enum that just represents all of these error cases, and potentially just unwraps the DxcOperationResult into this new enum.

Comment on lines +293 to +296
let mut linker_error = 0u32;
let status_hr = unsafe { result.get_status(&mut linker_error) };

if !hr.is_err() && !status_hr.is_err() && linker_error == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Like, how many errors do they want us to check, and at which point will DxcOperationResult::get_error_buffer() return sensible help text?

@Jasper-Bekkers Jasper-Bekkers enabled auto-merge (squash) October 20, 2023 08:28
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.

2 participants