-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix typing for select
#7572
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?
Fix typing for select
#7572
Conversation
991856b
to
cfd29df
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
cfd29df
to
c6dcb79
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
c6dcb79
to
62c8609
Compare
2de9a82
to
83ad8bb
Compare
This comment was marked as resolved.
This comment was marked as resolved.
select
select
db91e44
to
2f271ab
Compare
d792084
to
817989b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
6db2cc1
to
6e62829
Compare
6e62829
to
88fa2bb
Compare
|
||
let select_single_component = | ||
|this: &mut Self, reject_scalar, reject, accept, condition| { | ||
let accept = this.cast(accept, reject_scalar, span)?; |
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.
Migrated from #7602 (comment): @jimblandy was wondering if we really needed cast
here. So far as I can tell...yes.
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.
Ah, another issue: We apparently coerce 0
and non-zero numeric values to bool
with cast
. That makes select
accept strictly more things than it should in constant evaluation; is there a reason we should allow such a cast?
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.
@jimblandy: Poke on this.
dd40727
to
1881436
Compare
select
select
1881436
to
4c8503b
Compare
let diag_deets = | ||
|module: &ir::Module, ty: &ir::TypeInner, orig_expr| { | ||
( | ||
ctx.ast_expressions.get_span(orig_expr), |
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.
When we lower an AST expression to a Naga IR expression, we preserve its span. You can use ctx.get_expression_span(lowered_expr)
to get these spans, so there's no need to retain the foo_orig
handles.
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 had originally authored this PR using get_expression_span
on lowered expression handles, but I actually found gaps in the spans returned. For many runtime expressions in the tests I'm adding, we instead get Span::default()
instead of the expected spans. 😐
I'd be happy to figure this particular issue out with you, but I suspect that it will need further work that's orthogonal to this PR. I don't think this should be blocking.
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.
Implemented this change (outside this PR) and changed tests to demonstrate the regressions: c449dd0
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.
Ah, this seems to be because the span lookup for locals appears to try to use something other than the span of the identifier referring to those locals; writing some quick debug diagnostics (fe60016) against this shader:
fn select_arrays(which: bool) -> i32 {
var x: array<i32, 4>;
var y: array<i32, 4> = array(1, 2, 3, 4);
let s = select(x, y, which);
return s[0];
}
…yields this output:
error: before
┌─ wgsl:4:18
│
4 │ let s = select(x, y, which);
│ ^
error: after
error: before
┌─ wgsl:4:21
│
4 │ let s = select(x, y, which);
│ ^
error: after
error: before
┌─ wgsl:4:24
│
4 │ let s = select(x, y, which);
│ ^^^^^
error: after
┌─ wgsl:1:18
│
1 │ fn select_arrays(which: bool) -> i32 {
│ ^^^^^
Could not parse WGSL:
error: unexpected argument type for `select` call
= note: expected a scalar or a `vecN` of scalars
…where before
and after
diagnostics should be pointing to the same span, but they don't. If it's true that get_expression_span
should be returning the same span as the expression used to get the ir::Handle
, it looks like we have some nontrivial changes to make.
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.
This is a known limitation, filed as #7498. When we fix that, these diagnostics will magically get better.
You can work around it in your tests by making the operand more complex than a variable reference, like y + 0
or something.
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.
...but why would I accept bad diagnostics for something that's easy to migrate to once fixed? I'd strongly prefer that this code be migrated to get_expression_span
after it's fixed, because the diagnostics are okay right now. 😭
4c8503b
to
291d321
Compare
291d321
to
0da4827
Compare
Connections
Depends on feat(naga): constant evaluation forselect
#7602.select
#7538.Testing
Tests have been added, as well as a test run against
webgpu:shader,execution,expression,call,builtin,select:*
in a locally modified Firefox.Squash or Rebase?
rebase plz
Checklist
CHANGELOG.md
entry.