-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[glsl-in] fix missing side effects from sequence expressions #8787
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
base: trunk
Are you sure you want to change the base?
Changes from 8 commits
f6eacdf
984dc4f
72598bf
07f2b31
c15c7e3
6c23acc
3976fef
edcd4e3
c7901b2
9afc0b7
cbba6e2
09f508e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -511,20 +511,43 @@ impl ParsingContext<'_> { | |
| }) | ||
| } | ||
|
|
||
| // shader grammar :- | ||
| // expression : | ||
| // assignment_expression | ||
| // expression COMMA assignment_expression | ||
| pub fn parse_expression( | ||
| &mut self, | ||
| frontend: &mut Frontend, | ||
| ctx: &mut Context, | ||
| stmt: &mut StmtContext, | ||
| ) -> Result<Handle<HirExpr>> { | ||
| let mut exprs = Vec::new(); | ||
| let mut expr = self.parse_assignment(frontend, ctx, stmt)?; | ||
| exprs.push(expr); | ||
|
|
||
| while let TokenValue::Comma = self.expect_peek(frontend)?.value { | ||
| self.bump(frontend)?; | ||
| expr = self.parse_assignment(frontend, ctx, stmt)?; | ||
| exprs.push(expr); | ||
| } | ||
|
|
||
| Ok(expr) | ||
| if exprs.len() == 1 { | ||
| Ok(expr) | ||
| } else { | ||
| let mut meta = stmt.hir_exprs[exprs[0]].meta; | ||
| for &e in &exprs[1..] { | ||
| meta.subsume(stmt.hir_exprs[e].meta); | ||
| } | ||
|
Comment on lines
+534
to
+536
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for handling the spans correctly |
||
| expr = stmt.hir_exprs.append( | ||
| HirExpr { | ||
| kind: HirExprKind::Sequence { exprs }, | ||
| meta, | ||
| }, | ||
| Default::default(), | ||
| ); | ||
|
|
||
| Ok(expr) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lots of non-functional changes here, see overall review comment |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // issue #6208 https://github.com/gfx-rs/wgpu/issues/6208 | ||
| # version 460 | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be preferred to adopt webgl cts tests, for example https://registry.khronos.org/webgl/sdk/tests/conformance/glsl/bugs/sequence-operator-evaluation-order.html?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats a question that @teoxoy will probably answer |
||
|
|
||
| void main() { | ||
| float a = 1.0; | ||
| float b = 0.25; | ||
| float c = 1.5; | ||
| int i = 20; | ||
|
|
||
| // tests for multiple expressions in first part (if it's a expression, not declaration)! | ||
| // also the third part! | ||
| for (i = 0, c-=1.0; i < 25; i++, b+=0.01) { | ||
| a -= 0.02; | ||
| } | ||
|
|
||
| // a, b and c should be all ~0.5! | ||
| // right now it only ever takes the last expression in the block... | ||
| // leading to infinite loops, lost devices and incorrect results. | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| fn main_1() { | ||
| var a: f32 = 1f; | ||
| var b: f32 = 0.25f; | ||
| var c: f32 = 1.5f; | ||
| var i: i32 = 20i; | ||
|
|
||
| i = 0i; | ||
| let _e9 = c; | ||
| c = (_e9 - 1f); | ||
| loop { | ||
| let _e12 = i; | ||
| if !((_e12 < 25i)) { | ||
| break; | ||
| } | ||
| { | ||
| let _e22 = a; | ||
| a = (_e22 - 0.02f); | ||
| } | ||
| continuing { | ||
| let _e16 = i; | ||
| i = (_e16 + 1i); | ||
| let _e19 = b; | ||
| b = (_e19 + 0.01f); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| @fragment | ||
| fn main() { | ||
| main_1(); | ||
| return; | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the GLSL spec document it lists one change for 4.60, revision 4 section 1.1.4
the explanation between 4.50 and 4.60 (both times Section 5.9) are also slightly different.
The later version adding two new phrases:
It is unclear to me if you target a specific version, or if the translation should care.
Perhaps it's fine to use
lower_inner()instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit out of the loop on GLSL stuff. The parser is very much best-effort at this point. Its more about not breaking other people's code than implementing the GLSL spec in the most perfect way. So probably its your call what we should do here.