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

[spv-out] Fix duplicate scalar OpType #266

Merged
merged 1 commit into from Nov 13, 2020

Conversation

Napokue
Copy link
Collaborator

@Napokue Napokue commented Nov 10, 2020

When the following WGSL input was used:

[[stage(vertex)]]
fn main() -> void {
  var vector: vec2<f32> = vec2<f32>(1.0, 1.0);
  return;
}

You would get a validation error when validating the SPIR-V output. The reason was because there would be a duplicate OpTypeFloat is because the vector implicitly uses OpTypeFloat here.

If you would first explicity add a float variable, it would work:

[[stage(vertex)]]
fn main() -> void {
  var float: f32 = 1.0;
  var vector: vec2<f32> = vec2<f32>(1.0, 1.0);
  return;
}

The reason this works is as there is an explicit f32 declaration and that is getting parsed correctly.

Technical

This is due to how LookupTypes work and I am not entirely sure how it is caused, but it something has to do that the implicit f32 don't have a handle and aren't recognized when looking up.

Conclusion

For now, this "quick" fix should push the code forward, so we can focus on getting in more support for more shaders. The Lookup system works fine for now, but it can use some refactor a little later down the road.

This PR is one step closer to successfully parse the quad.wgsl

@Napokue Napokue requested a review from kvark November 10, 2020 20:00
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you for identifying and fixing this! I believe the fix is in the right direction.
The whole lookup_type set of guarantees is a bit messy still, since it's modified in multiple places, but refactoring this has to wait for a better time.

src/back/spv/writer.rs Outdated Show resolved Hide resolved
src/back/spv/writer.rs Show resolved Hide resolved
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

ok, postponing the naming changes

@Napokue Napokue merged commit a1dc8c2 into gfx-rs:master Nov 13, 2020
@Napokue Napokue deleted the duplicate-scalar-type branch November 13, 2020 18:55
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.

None yet

2 participants