Skip to content

Add function restrictions #1674

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.

Sign up for GitHub

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

Jump to bottom
Merged
dneto0 merged 5 commits into from
May 6, 2021
Merged

Add function restrictions #1674

dneto0 merged 5 commits into from
May 6, 2021

Conversation

@alan-baker
Copy link
Contributor

@alan-baker alan-baker commented Apr 26, 2021

Fixes #1567, #1471, #1457, #1139

  • No recursion
  • No calling entry points
  • fragment shader must return position
  • aliasing restrictions
  • pointer restrictions

The allowance handle pointer parameters is somewhat debatable, but I've included it since we don't have a clear alternative at the moment.

@alan-baker
Add function restrictions
620ebf9 
Fixes gpuweb#1567, gpuweb#1471, gpuweb#1457, gpuweb#1139

* No recursion
* No calling entry points
* fragment shader must return position
* aliasing restrictions
* pointer restrictions
@alan-baker alan-baker added the wgsl WebGPU Shading Language Issues label Apr 26, 2021
@alan-baker alan-baker requested review from dneto0, kvark and litherum April 26, 2021 19:00
@github-actions
Copy link
Contributor

github-actions bot commented Apr 26, 2021

Previews, as seen when this build job started (620ebf9):
WebGPU | IDL
WGSL
Explainer

jrprice
jrprice reviewed Apr 27, 2021
View reviewed changes
@github-actions
Copy link
Contributor

github-actions bot commented Apr 27, 2021

Previews, as seen when this build job started (472f231):
WebGPU | IDL
WGSL
Explainer

dneto0
dneto0 requested changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@dneto0 dneto0 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.

I've got two nits.

@alan-baker
Changes for review
42686c3 
* clarify possible return types to disallow handles
* clarify aliasing
RobinMorisset
RobinMorisset reviewed Apr 27, 2021
View reviewed changes
@kdashg
Copy link
Contributor

kdashg commented Apr 27, 2021

WGSL meeting minutes 2021-04-27
  • AB: The contentious part is whether you can pass a handle by pointer. I left that in. Otherwise, the restrictions reflect basic SPIR-V.
  • MM: At office hours, discussed that maybe if we allow texture pointers, it might break some of the analysis we want to do
  • DN: For texture filtering? (yes) How does that leak into WGSL?
  • MM: If possible to pass by ref, at the point that you compile, the pipeline layout tells you the inner type of the texture and whether it’s sampleable. If textures are named specifically, we can do this validation.
  • DM: Yes we need to know what textures are used by which samplers
  • MM: Can we add more types for this?
  • DM: We had that proposal earlier, and we didn’t want that at that time
  • DN: What’s proposal for passing tex/samplers to functions?
  • MM: By pointer would be great. If we want to keep the analysis, I think we wouldn’t be able to do this, rather you’d have to name it. I wish we could fix this. Statically determining tex/sampler at compiler time is valuable.
  • KN: I thought it wasn’t just analysis, but also some hardware needs to be able to determine this.
  • JG: Well, spirv isn’t really that flexible. Can determine static-use
  • KN: Could do e.g. unrolling to figure these out
  • MM: Let’s see if we can figure this out offline
  • DN: Can we land this without the contested bits?

This was referenced Apr 29, 2021
Closed
Merged
@alan-baker alan-baker mentioned this pull request May 6, 2021
Closed
dneto0
dneto0 approved these changes May 6, 2021
View reviewed changes
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@dneto0 dneto0 merged commit 73966b5 into gpuweb:main May 6, 2021
@alan-baker alan-baker mentioned this pull request May 6, 2021
Merged
@alan-baker alan-baker deleted the function-restrictions branch August 20, 2021 14:14
@kdashg kdashg mentioned this pull request Sep 29, 2021
Closed
@dneto0 dneto0 mentioned this pull request Nov 29, 2021
Closed
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
@Gyuyoung
Enhance val:render_pass_descriptor:depth_stencil_attachment_depth_cle…
d5e662b 
…ar_value (gpuweb#1674)

This PR updates render_pass_descriptor:depth_stencil_attachment_depth_clear_value
to check if out of range of depthClearValue is only invalid when depthLoadOp is
`clear'. If depthLoadOp is not `clear`, depthClearValue should be valid even
though the value is out of range.

Issue: gpuweb#1618
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RobinMorisset RobinMorisset RobinMorisset left review comments

@jrprice jrprice jrprice left review comments

@kvark kvark kvark approved these changes

@dneto0 dneto0 dneto0 approved these changes

@litherum litherum Awaiting requested review from litherum

Assignees

No one assigned

Labels

wgsl WebGPU Shading Language Issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Require that vertex shaders return a position?

6 participants

@alan-baker @kdashg @kvark @RobinMorisset @jrprice @dneto0