[naga] Add phony as naga-ir statement #6308
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
|
|
All So I think all new FAILs were actually fake PASS. |
There was a problem hiding this comment.
LGTM, minus the questions I have. The only other person in @gfx-rs/naga that I know is on-duty for this is @jimblandy, though, so I'll ask for his review explicitly.
Comment on lines
+1605
to
+1614
| Statement::Phony(expr) => { | ||
| write!(self.out, "{level}")?; | ||
| self.write_named_expr( | ||
| module, | ||
| expr, | ||
| format!("_phony_{}", expr.index()), | ||
| expr, | ||
| func_ctx, | ||
| )?; | ||
| } |
There was a problem hiding this comment.
question: @gfx-rs/naga, should we be using self.namer here? I think so, since I don't think this logic has any guarantee of a hygienic name?
There was a problem hiding this comment.
The catch, as hinted at in #6095 (comment), is that even ignored expressions affect WGSL's rules for which declarations are statically accessed. In other words, when you've got
let _ = buffer;
in some function body, according to WGSL's rules, that means that buffer is "statically accessed" by that function - even though the expression is ignored.
Comment on lines
+2933
to
+2934
| self.start_baking_expression(expr, &context.expression, &name)?; | ||
| self.put_expression(expr, &context.expression, true)?; |
There was a problem hiding this comment.
question: I actually don't know when we should start baking expressions; this is something I need to learn about Naga internals, too.
@gfx-rs/naga, should we be baking expressions here? I presume so, since we may have an arbitrary expression on the RHS of a phony assignment.
Comment on lines
+838
to
+847
| if self | ||
| .flags | ||
| .contains(ValidationFlags::CONTROL_FLOW_UNIFORMITY) | ||
| && !req.is_empty() | ||
| { | ||
| if let Some(cause) = disruptor { | ||
| return Err(FunctionError::NonUniformControlFlow(req, expr, cause) | ||
| .with_span_handle(expr, expression_arena)); | ||
| } | ||
| } |
There was a problem hiding this comment.
question: @gfx-rs/naga, does this look right? I'm not sure, since AFAIK the RHS of an assignment shouldn't have any visible effects. Therefore, it shouldn't matter if we have any uniformity obligations here, maybe?
ErichDonGubler
added
area: correctness
jimblandy
requested changes
Sep 25, 2024
There was a problem hiding this comment.
I'm actually somewhat concerned here that the whole compaction process is going to be running into this kind of problem all over the place.
Compaction was originally introduced to help out with constant evaluation: we wanted the validator to simply reject modules that contained any abstract types, but the constant evaluator tended to leave expressions with abstract types lying around in the arenas. The idea was that compaction would reduce the module to "the parts that actually matter", and all traces of the module's state before constant evaluation would be gone.
I'm concerned that let _ = statements are only one manifestation of this problem: we can fix it here, but as long as we have compaction, we'll have to discover and remove other situations case by case.
This PR certainly fixes a lot of failures, but I think the right approach might simply be to change the validator to permit abstract types in the type arena, and only reject them when they are used as globals' types, local var types, components of structs, etc. Then the ordinary type rules should prevent abstract types from appearing within expressions. Then we could remove compaction altogether; I don't think we need it for anything else, and it's complex.
@sagudev, does that make sense? Is that a project you'd be willing to take on?
If not, a simpler approach to this bug might be to simply have the WGSL front end introduce an anonymous local variable, and have the let _ store the expression's value in there, like a let x would.
In either case, I would rather not have the new Statement variant.
|
Another possible example of where compaction goes awry: I'm not sure, but I suspect the WGSL spec requires us to issue an error if derivative operations are used in a compute shader. But we could say |
I think compacting also removes unused expressions, maybe it does even more I am not really that familiar, but I know that WGSL specifically: wgpu/naga/src/front/wgsl/lower/mod.rs Lines 1224 to 1227 in e7f891b
I think this is way over my head.
Initially I was actually trying to do this, but I was scared that compacting will eliminate some btw WGSL doesn't have |
6 tasks
|
Closing this in favor of #6328 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connections
Fix #6095
Description
Currently phony statements get removed when compacting and when emitting, we can solve both problems by introducing new phony statement that are not compacted away and are always emitted.
Why not reusing emit? Because it makes reasoning easier (we would need to somehow encode phonies in emit statements and this gets very ugly) and phonies are just different enough to deserve special treatment. Also per emit docs:
Emit a range of expressions, visible to all statements that follow in this block, but phony statements are not visible to other statements. This was also discussed in gfx-rs/naga#1866 (comment), but naga changed (and regressed) as compacting was introduced, so instead of hacking around more I think this is the most elegant solution.Testing
Existing tests, new tests and servo CTS run.
Checklist
cargo fmt.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.