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

glsl-out: Implement bounds checks for ImageLoad #1889

Merged
merged 3 commits into from
May 30, 2022

Conversation

JCapucho
Copy link
Collaborator

@JCapucho JCapucho commented May 3, 2022

Depends on #1887

Implements bounds checks for ImageLoad on sampled images, because the opengl behavior on invalid texel load and stores for storage images is ReadZeroSkipWrite (no bound checks needed).

Only works on core for now since es doesn't support samples/levels queries (kvark said in the matrix room that they should be passed in uniforms then)

Closes #1080

@JCapucho JCapucho force-pushed the glsl-out-bounds-checks branch 3 times, most recently from f67e544 to d5f6c6f Compare May 5, 2022 18:10
@JCapucho JCapucho changed the title Implement bounds checks for ImageLoad glsl-out: Implement bounds checks for ImageLoad May 5, 2022
@JCapucho JCapucho requested a review from jimblandy May 5, 2022 18:10
@jimblandy
Copy link
Member

@JCapucho Should we enable GLSL output for the tests/in/bounds-check-image-* snapshots, in this PR?

@jimblandy
Copy link
Member

I guess we'd have to add an actual entry point to the WGSL file, and have it call everything.

@JCapucho JCapucho force-pushed the glsl-out-bounds-checks branch 2 times, most recently from 009ed82 to ad876df Compare May 28, 2022 21:31
In addition to the snapshot.rs changes, this entails adding an entry
point function to `bounds-check-image-restrict.wgsl` and
`bounds-check-image-rzsw.wgsl`, including appropriate data in the
param.ron files.
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

This looks great - thank you! I appreciated the comments; especially in write_image_load there are a lot of different things going on and the code gets long, and it helped me keep score.

These are all just suggestions.

src/back/glsl/features.rs Show resolved Hide resolved
src/back/glsl/mod.rs Show resolved Hide resolved
src/back/glsl/mod.rs Show resolved Hide resolved
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
src/back/glsl/mod.rs Outdated Show resolved Hide resolved
@JCapucho JCapucho requested a review from jimblandy May 29, 2022 22:05
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Looks great - thank you!

@jimblandy jimblandy merged commit 0aa7681 into gfx-rs:master May 30, 2022
jimblandy added a commit to jimblandy/naga that referenced this pull request Jun 1, 2022
As of gfx-rs#1889, the GLSL back end takes an additional argument specifying
the bounds checks policies to use.
jimblandy added a commit that referenced this pull request Jun 1, 2022
As of #1889, the GLSL back end takes an additional argument specifying
the bounds checks policies to use.
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.

textureLoad and textureStore need bounds checks
2 participants