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

Add missing float image format inference #5716

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Devon7925
Copy link

fixes #5704

@Devon7925 Devon7925 requested a review from a team as a code owner December 1, 2024 21:33
@CLAassistant
Copy link

CLAassistant commented Dec 1, 2024

CLA assistant check
All committers have signed the CLA.

csyonghe
csyonghe previously approved these changes Dec 1, 2024
@csyonghe
Copy link
Collaborator

csyonghe commented Dec 1, 2024

Looks good to me. Thank you for your contribution!

@csyonghe csyonghe added the pr: non-breaking PRs without breaking changes label Dec 1, 2024
@csyonghe
Copy link
Collaborator

csyonghe commented Dec 1, 2024

There is a test whose expected result needs to be updated.

@Devon7925
Copy link
Author

Not sure how to do that.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 2, 2024

Actually the test failed because the generated spirv failed validation. We need to investigate which validation rule is being violated.

@Devon7925
Copy link
Author

On my computer that test doesn't fail so I'm not sure how to proceed.

@jkwak-work
Copy link
Collaborator

On my computer that test doesn't fail so I'm not sure how to proceed.

You probably didn't enable the SPIRV validation.
We documented the instruction in https://github.com/shader-slang/slang/blob/master/CONTRIBUTING.md#testing
In short, you should set an environment variable as following,

set SLANG_RUN_SPIRV_VALIDATION=1

@Devon7925
Copy link
Author

The test fails due to this line
%sampledImageType = OpTypeSampledImage $$Texture2D<float>;
It still infers unknown for the texture format and that doesn't match the newly inferred type of the texture. The test also fails independently of this pr if half is used instead of float.

I've been unable to figure out where to put the inference code though I'm still looking.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 3, 2024

Ah, actually we do want to have Texture2D<float> or Texture2D<float4> to map to unknown format when generating SPIRV, so we shouldn't be adding the image format inference logic here.

The reasoning is that when the user defines something like Texture2D texture; without a format attribute, it automatically expands to Texture2D<float4> texture;, and in this case we shouldn't produce SPIRV code that declares the format to be RGBA32F. Most often the texture will NOT be in that format and instead be something like rgba8unorm. Specifying unknown here is the only correct behavior.

@Devon7925
Copy link
Author

I could infer for float but not float4 if the issue is just the Texture2D to Texture2D<float4> conversion which would fix the issue in wgsl. Would that work?

@Devon7925
Copy link
Author

It would require modifying the test to use float4 instead of float, but since the test doesn't work with half, it is already testing a special case.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 3, 2024

Inferring for float maybe OK, since it is a less common case. But still the argument applies for R8UNORM formats.

It is safer to not infer for float, because all unorm/snorm formats can map to float.

The right fix to this problem IMO, is to allow user to write Texture2D<r8unorm> to be more explicit on the format, but t his requires some refactoring on how texture types are represented internally.

@Devon7925
Copy link
Author

People can use attributes to specify format as well. R10G10B10A2_UINT creates ambiguity for UInt4. If we want to prevent ever inferring the wrong format maybe removing format inference all together would be better. I would argue getting rgba32 for float in wgsl due to not inferring is just as bad as getting other undesired formats due to inferring.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 4, 2024

For WGSL where there isn't a "unknown" format, I agree that any inference is better.
But we need to be careful to not break SPIRV applications by adding more inferencing.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 4, 2024

Perhaps we should do this during wgsl emit time, so we can do a last minute infer for wgsl when the default front-end infer failed.

csyonghe
csyonghe previously approved these changes Dec 4, 2024
@Devon7925
Copy link
Author

Perhaps we should do this during wgsl emit time, so we can do a last minute infer for wgsl when the default front-end infer failed.

Looks like that's what glsl does. Converted over to using that.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 4, 2024

Looks good, need a formatting pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiling RWTexture2D<float> to wgsl leads to rgba32float instead of r32float
4 participants