Skip to content

Commit

Permalink
glsl-in: Fix memory qualifiers being inverted
Browse files Browse the repository at this point in the history
Adds some documentation to better explain how the memory qualifier works
troughout the parser and some storage textures tests.
  • Loading branch information
JCapucho committed Mar 19, 2022
1 parent f90e563 commit 3554c19
Show file tree
Hide file tree
Showing 7 changed files with 115 additions and 51 deletions.
2 changes: 2 additions & 0 deletions src/front/glsl/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ pub struct TypeQualifiers<'a> {
pub interpolation: Option<(Interpolation, Span)>,
pub precision: Option<(Precision, Span)>,
pub sampling: Option<(Sampling, Span)>,
/// Memory qualifiers used in the declaration to set the storage access to be used
/// in declarations that support it (storage images and buffers)
pub storage_acess: Option<(StorageAccess, Span)>,
pub layout_qualifiers: crate::FastHashMap<QualifierKey<'a>, (QualifierValue, Span)>,
}
Expand Down
4 changes: 2 additions & 2 deletions src/front/glsl/lex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ impl<'a> Iterator for Lexer<'a> {
"mediump" => TokenValue::PrecisionQualifier(Precision::Medium),
"lowp" => TokenValue::PrecisionQualifier(Precision::Low),
"restrict" => TokenValue::Restrict,
"readonly" => TokenValue::StorageAccess(StorageAccess::LOAD),
"writeonly" => TokenValue::StorageAccess(StorageAccess::STORE),
"readonly" => TokenValue::MemoryQualifier(StorageAccess::LOAD),
"writeonly" => TokenValue::MemoryQualifier(StorageAccess::STORE),
// values
"true" => TokenValue::BoolConstant(true),
"false" => TokenValue::BoolConstant(false),
Expand Down
10 changes: 5 additions & 5 deletions src/front/glsl/parser/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl<'source> ParsingContext<'source> {
| TokenValue::Shared
| TokenValue::Buffer
| TokenValue::Restrict
| TokenValue::StorageAccess(_)
| TokenValue::MemoryQualifier(_)
| TokenValue::Layout => true,
_ => false,
})
Expand Down Expand Up @@ -219,11 +219,11 @@ impl<'source> ParsingContext<'source> {

qualifiers.precision = Some((p, token.meta));
}
TokenValue::StorageAccess(access) => {
TokenValue::MemoryQualifier(access) => {
let storage_access = qualifiers
.storage_acess
.get_or_insert((crate::StorageAccess::empty(), Span::default()));
if storage_access.0.contains(access) {
.get_or_insert((crate::StorageAccess::all(), Span::default()));
if !storage_access.0.contains(!access) {
parser.errors.push(Error {
kind: ErrorKind::SemanticError(
"The same memory qualifier can only be used once".into(),
Expand All @@ -232,7 +232,7 @@ impl<'source> ParsingContext<'source> {
})
}

storage_access.0 |= access;
storage_access.0 &= access;
storage_access.1.subsume(token.meta);
}
TokenValue::Restrict => continue,
Expand Down
6 changes: 5 additions & 1 deletion src/front/glsl/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ pub enum TokenValue {
Shared,

Restrict,
StorageAccess(crate::StorageAccess),
/// A `glsl` memory qualifier such as `writeonly`
///
/// The associated [`crate::StorageAccess`] is the access being allowed
/// (for example `writeonly` has an associated value of [`crate::StorageAccess::STORE`])
MemoryQualifier(crate::StorageAccess),

Interpolation(Interpolation),
Sampling(Sampling),
Expand Down
10 changes: 4 additions & 6 deletions src/front/glsl/variables.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,8 @@ impl Parser {
StorageQualifier::AddressSpace(mut space) => {
match space {
AddressSpace::Storage { ref mut access } => {
if let Some((restricted_access, _)) = qualifiers.storage_acess.take() {
access.remove(restricted_access);
if let Some((allowed_access, _)) = qualifiers.storage_acess.take() {
*access = allowed_access;
}
}
AddressSpace::Uniform => match self.module.types[ty].inner {
Expand All @@ -480,10 +480,8 @@ impl Parser {
mut format,
} = class
{
if let Some((restricted_access, _)) =
qualifiers.storage_acess.take()
{
access.remove(restricted_access);
if let Some((allowed_access, _)) = qualifiers.storage_acess.take() {
access = allowed_access;
}

match qualifiers.layout_qualifiers.remove(&QualifierKey::Format) {
Expand Down
18 changes: 18 additions & 0 deletions tests/in/glsl/images.frag
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ layout(rgba8, binding = 4) uniform image1DArray img1DArray;
layout(rgba8, binding = 5) uniform image2DArray img2DArray;
// layout(rgba8, binding = 6) uniform imageCubeArray imgCubeArray;

layout(rgba8, binding = 7) readonly uniform image2D imgReadOnly;
layout(rgba8, binding = 8) writeonly uniform image2D imgWriteOnly;
layout(rgba8, binding = 9) writeonly readonly uniform image2D imgWriteReadOnly;

void testImg1D(in int coord) {
int size = imageSize(img1D);
vec4 c = imageLoad(img1D, coord);
Expand Down Expand Up @@ -52,4 +56,18 @@ void testImg3D(in ivec3 coord) {
// imageStore(imgCubeArray, coord, vec4(2));
// }

void testImgReadOnly(in ivec2 coord) {
vec2 size = imageSize(img2D);
vec4 c = imageLoad(imgReadOnly, coord);
}

void testImgWriteOnly(in ivec2 coord) {
vec2 size = imageSize(img2D);
imageStore(imgWriteOnly, coord, vec4(2));
}

void testImgWriteReadOnly(in ivec2 coord) {
vec2 size = imageSize(imgWriteReadOnly);
}

void main() {}
116 changes: 79 additions & 37 deletions tests/out/wgsl/images-frag.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,26 @@ var img3D: texture_storage_3d<rgba8unorm,read_write>;
var img1DArray: texture_storage_1d_array<rgba8unorm,read_write>;
@group(0) @binding(5)
var img2DArray: texture_storage_2d_array<rgba8unorm,read_write>;
@group(0) @binding(7)
var imgReadOnly: texture_storage_2d<rgba8unorm,read>;
@group(0) @binding(8)
var imgWriteOnly: texture_storage_2d<rgba8unorm,write>;
@group(0) @binding(9)
var imgWriteReadOnly: texture_storage_2d<rgba8unorm,write>;

fn testImg1D(coord: i32) {
var coord_1: i32;
var size: i32;
var c: vec4<f32>;

coord_1 = coord;
let _e7 = textureDimensions(img1D);
size = _e7;
let _e10 = coord_1;
let _e11 = textureLoad(img1D, _e10);
c = _e11;
let _e17 = coord_1;
textureStore(img1D, _e17, vec4<f32>(f32(2)));
let _e10 = textureDimensions(img1D);
size = _e10;
let _e13 = coord_1;
let _e14 = textureLoad(img1D, _e13);
c = _e14;
let _e20 = coord_1;
textureStore(img1D, _e20, vec4<f32>(f32(2)));
return;
}

Expand All @@ -31,14 +37,14 @@ fn testImg1DArray(coord_2: vec2<i32>) {
var c_1: vec4<f32>;

coord_3 = coord_2;
let _e7 = textureDimensions(img1DArray);
let _e8 = textureNumLayers(img1DArray);
size_1 = vec2<f32>(vec2<i32>(_e7, _e8));
let _e13 = coord_3;
let _e16 = textureLoad(img1DArray, _e13.x, _e13.y);
c_1 = _e16;
let _e22 = coord_3;
textureStore(img1DArray, _e22.x, _e22.y, vec4<f32>(f32(2)));
let _e10 = textureDimensions(img1DArray);
let _e11 = textureNumLayers(img1DArray);
size_1 = vec2<f32>(vec2<i32>(_e10, _e11));
let _e16 = coord_3;
let _e19 = textureLoad(img1DArray, _e16.x, _e16.y);
c_1 = _e19;
let _e25 = coord_3;
textureStore(img1DArray, _e25.x, _e25.y, vec4<f32>(f32(2)));
return;
}

Expand All @@ -48,13 +54,13 @@ fn testImg2D(coord_4: vec2<i32>) {
var c_2: vec4<f32>;

coord_5 = coord_4;
let _e7 = textureDimensions(img2D);
size_2 = vec2<f32>(_e7);
let _e11 = coord_5;
let _e12 = textureLoad(img2D, _e11);
c_2 = _e12;
let _e18 = coord_5;
textureStore(img2D, _e18, vec4<f32>(f32(2)));
let _e10 = textureDimensions(img2D);
size_2 = vec2<f32>(_e10);
let _e14 = coord_5;
let _e15 = textureLoad(img2D, _e14);
c_2 = _e15;
let _e21 = coord_5;
textureStore(img2D, _e21, vec4<f32>(f32(2)));
return;
}

Expand All @@ -64,14 +70,14 @@ fn testImg2DArray(coord_6: vec3<i32>) {
var c_3: vec4<f32>;

coord_7 = coord_6;
let _e7 = textureDimensions(img2DArray);
let _e10 = textureNumLayers(img2DArray);
size_3 = vec3<f32>(vec3<i32>(_e7.x, _e7.y, _e10));
let _e15 = coord_7;
let _e18 = textureLoad(img2DArray, _e15.xy, _e15.z);
c_3 = _e18;
let _e24 = coord_7;
textureStore(img2DArray, _e24.xy, _e24.z, vec4<f32>(f32(2)));
let _e10 = textureDimensions(img2DArray);
let _e13 = textureNumLayers(img2DArray);
size_3 = vec3<f32>(vec3<i32>(_e10.x, _e10.y, _e13));
let _e18 = coord_7;
let _e21 = textureLoad(img2DArray, _e18.xy, _e18.z);
c_3 = _e21;
let _e27 = coord_7;
textureStore(img2DArray, _e27.xy, _e27.z, vec4<f32>(f32(2)));
return;
}

Expand All @@ -81,13 +87,49 @@ fn testImg3D(coord_8: vec3<i32>) {
var c_4: vec4<f32>;

coord_9 = coord_8;
let _e7 = textureDimensions(img3D);
size_4 = vec3<f32>(_e7);
let _e11 = coord_9;
let _e12 = textureLoad(img3D, _e11);
c_4 = _e12;
let _e18 = coord_9;
textureStore(img3D, _e18, vec4<f32>(f32(2)));
let _e10 = textureDimensions(img3D);
size_4 = vec3<f32>(_e10);
let _e14 = coord_9;
let _e15 = textureLoad(img3D, _e14);
c_4 = _e15;
let _e21 = coord_9;
textureStore(img3D, _e21, vec4<f32>(f32(2)));
return;
}

fn testImgReadOnly(coord_10: vec2<i32>) {
var coord_11: vec2<i32>;
var size_5: vec2<f32>;
var c_5: vec4<f32>;

coord_11 = coord_10;
let _e10 = textureDimensions(img2D);
size_5 = vec2<f32>(_e10);
let _e14 = coord_11;
let _e15 = textureLoad(imgReadOnly, _e14);
c_5 = _e15;
return;
}

fn testImgWriteOnly(coord_12: vec2<i32>) {
var coord_13: vec2<i32>;
var size_6: vec2<f32>;

coord_13 = coord_12;
let _e10 = textureDimensions(img2D);
size_6 = vec2<f32>(_e10);
let _e17 = coord_13;
textureStore(imgWriteOnly, _e17, vec4<f32>(f32(2)));
return;
}

fn testImgWriteReadOnly(coord_14: vec2<i32>) {
var coord_15: vec2<i32>;
var size_7: vec2<f32>;

coord_15 = coord_14;
let _e10 = textureDimensions(imgWriteReadOnly);
size_7 = vec2<f32>(_e10);
return;
}

Expand Down

0 comments on commit 3554c19

Please sign in to comment.