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

Allow array_index to be unsigned #2298

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Apr 2, 2023

Following the spec, functions with the parameter array_index, e.g. in combination with texture_2d_array, should be able to take a u32 as well:
https://www.w3.org/TR/WGSL/#textureload
https://www.w3.org/TR/WGSL/#texturesample
https://www.w3.org/TR/WGSL/#texturestore

(I have no clue how to link to a more permanent version of the spec)

@daxpedda
Copy link
Contributor Author

daxpedda commented Apr 2, 2023

Alright, this is far outside my comfort zone. Any help would be appreciated.

I assume that either some code was relying on this value being a i32 and has to be adjusted to take that into account or SPIR-V just doesn't allow a i32 in this case.

@teoxoy
Copy link
Member

teoxoy commented Apr 4, 2023

SPIR-V requires a vector containing the coordinates and the array index. The type of the vector has to match with the type of the scalars that compose it and it's here that we assume the array_index is signed:

naga/src/back/spv/image.rs

Lines 317 to 352 in 7c00548

let array_index_i32_id = self.cached[array_index];
let reconciled_array_index_id = if component_kind == crate::ScalarKind::Sint {
array_index_i32_id
} else if component_kind == crate::ScalarKind::Uint {
let u32_id = self.get_type_id(LookupType::Local(LocalType::Value {
vector_size: None,
kind: crate::ScalarKind::Uint,
width: 4,
pointer_space: None,
}));
let reconciled_id = self.gen_id();
block.body.push(Instruction::unary(
spirv::Op::Bitcast,
u32_id,
reconciled_id,
array_index_i32_id,
));
reconciled_id
} else {
let component_type_id = self.get_type_id(LookupType::Local(LocalType::Value {
vector_size: None,
kind: component_kind,
width: 4,
pointer_space: None,
}));
let reconciled_id = self.gen_id();
block.body.push(Instruction::unary(
spirv::Op::ConvertUToF,
component_type_id,
reconciled_id,
array_index_i32_id,
));
reconciled_id
};

src/back/spv/image.rs Outdated Show resolved Hide resolved
@daxpedda
Copy link
Contributor Author

daxpedda commented Apr 4, 2023

@teoxoy thank you for the help. I hope I didn't go overkill on the checks here.
Happy to take advice on any code/style preferences you use in this repo.

@daxpedda daxpedda marked this pull request as ready for review April 4, 2023 13:06
src/back/spv/image.rs Outdated Show resolved Hide resolved
src/back/spv/image.rs Outdated Show resolved Hide resolved
@teoxoy
Copy link
Member

teoxoy commented Apr 4, 2023

@teoxoy thank you for the help. I hope I didn't go overkill on the checks here. Happy to take advice on any code/style preferences you use in this repo.

No problem :)
Could you also add a few more tests for this?

@daxpedda
Copy link
Contributor Author

daxpedda commented Apr 4, 2023

Could you also add a few more tests for this?

Could you point me in any specific direction, or do you have something in mind?

@daxpedda daxpedda requested a review from teoxoy April 4, 2023 20:27
@gfx-rs gfx-rs deleted a comment from codecov-commenter Apr 5, 2023
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the comments - looks great!

Could you also add a few more tests for this?

Could you point me in any specific direction, or do you have something in mind?

It seems the only test updated was textureLoad. I think we should at least test textureStore and textureSample as well. The other texture sampling functions use the same code paths so we might not need to test those.

tests/in/binding-arrays.wgsl Outdated Show resolved Hide resolved
@daxpedda
Copy link
Contributor Author

daxpedda commented Apr 6, 2023

Added a bunch more tests, hope that's not overkill.

@daxpedda daxpedda requested a review from teoxoy April 6, 2023 08:48
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