Skip to content

Add 'require()' to interface in print.slang #301

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kaizhangNV
Copy link
Contributor

@kaizhangNV kaizhangNV commented Jun 17, 2025

Close shader-slang/slang#6764

Slang require to put require() capability keyword on the interface, because we can not propagate the requirement from the implementation to interface. So the shader needs to be changed.

We need to develop mechanism for Metal specific to input the buffer length instead of using GetDimensions method to make this working on Metal.

Close #6764.

Slang require to put require() capability keyword on the interface,
because we can not propagate the requirement from the implementation to
interface. So the shader needs to be changed.

We need to develop mechanism for Metal specific to input the buffer
length instead of using GetDimensions method to make this working on Metal.
@ccummingsNV
Copy link
Contributor

ccummingsNV commented Jun 18, 2025

Change looks clean/simple enough but I don't fully understand the implications of 'require' as a keyword. Does this just cause metal to throw an error rather than something nastier when you try to use print? (I'm happy for you to push this if so).

@kaizhangNV
Copy link
Contributor Author

kaizhangNV commented Jun 18, 2025

Correct, it just report capability error when targeting Metal, no impact on other platforms.

@skallweitNV
Copy link
Contributor

I think this is a workaround and should be clearly marked as such. Slang should error out when using GetDimensions on Metal and not just silently emit that to MSL. I don't understand where exactly the problem is why the capability system cannot accomplish that right now, but that's clearly an issue in Slang IMO. So I would just ask to clearly state that in the comment, ideally with a link to the GitHub issue describing the problem. It feels like this is a substantial shortcoming of the capability system, any timeline of when this can be fixed?

@skallweitNV
Copy link
Contributor

Is there a ticket for the issue with the capability system? I can only find these two tickets:

@kaizhangNV
Copy link
Contributor Author

kaizhangNV commented Jun 19, 2025

The reason that current cap system cannot capture this is that capability check happens at type checking stage. But all the target related stuff happening at IR stages. Note we cannot check any target related stuff in type checking stage, because the AST has to be totally target independent, otherwise, slang module/extensions cannot be used at all.

But can we do this in IR? Maybe we can, but it will be very messy, because in IR, there is no concept of interface/struct, etc. And you will loss all of those requirement information, the only thing you can do will be some ad-hoc method or another ad-hoc legalization pass to detect such kind of violation. So I think that is why we don't do that in IR at first place.

I don't think this PR is a WAR, it is just how people should use capability system. But this doesn't achieve what you need here I agree.

@kaizhangNV
Copy link
Contributor Author

kaizhangNV commented Jun 19, 2025

It feels like this is a substantial shortcoming of the capability system, any timeline of when this can be fixed?

I have no timeline on this, as it's not an easy fix. But we can create a task, and I can have some conversion with other folks to see if there is any alternative solutions.

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.

[Diagnostic] Metal: Invalid code generation
3 participants