Skip to content

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

Draft
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Apr 18, 2025

Connections

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

  • If this contains user-facing changes, add a CHANGELOG.md entry.

@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch from 991856b to cfd29df Compare April 18, 2025 20:48
@jimblandy

This comment was marked as resolved.

@jimblandy

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch from cfd29df to c6dcb79 Compare April 19, 2025 03:38
@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler

This comment was marked as resolved.

@jimblandy

This comment was marked as resolved.

@jimblandy

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch from c6dcb79 to 62c8609 Compare April 21, 2025 15:56
@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch 2 times, most recently from 2de9a82 to 83ad8bb Compare April 23, 2025 16:07
@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler changed the title WIP: fix(naga): properly impl. auto. type conv. for select fix(naga): properly impl. auto. type conv. for select Apr 23, 2025
@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch 2 times, most recently from db91e44 to 2f271ab Compare April 29, 2025 02:15
jimblandy

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch 2 times, most recently from d792084 to 817989b Compare April 29, 2025 20:18
@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch 2 times, most recently from 6db2cc1 to 6e62829 Compare May 1, 2025 21:14
@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch from 6e62829 to 88fa2bb Compare May 1, 2025 22:04

let select_single_component =
|this: &mut Self, reject_scalar, reject, accept, condition| {
let accept = this.cast(accept, reject_scalar, span)?;
Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jimblandy: Poke on this.

@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch 2 times, most recently from dd40727 to 1881436 Compare May 2, 2025 01:39
@ErichDonGubler ErichDonGubler marked this pull request as ready for review May 2, 2025 01:40
@ErichDonGubler ErichDonGubler changed the title fix(naga): properly impl. auto. type conv. for select Fix typing for select May 2, 2025
@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch from 1881436 to 4c8503b Compare May 2, 2025 01:45
jimblandy

This comment was marked as resolved.

let diag_deets =
|module: &ir::Module, ty: &ir::TypeInner, orig_expr| {
(
ctx.ast_expressions.get_span(orig_expr),
Copy link
Member

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.

Copy link
Member Author

@ErichDonGubler ErichDonGubler May 22, 2025

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member

@jimblandy jimblandy May 22, 2025

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.

Copy link
Member Author

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. 😭

@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch from 4c8503b to 291d321 Compare May 22, 2025 15:49
@ErichDonGubler ErichDonGubler force-pushed the auto-type-conv-for-select-args branch from 291d321 to 0da4827 Compare May 22, 2025 15:52
@ErichDonGubler ErichDonGubler requested review from crowlKats and a team as code owners May 22, 2025 16:38
@ErichDonGubler

This comment was marked as resolved.

@cwfitzgerald cwfitzgerald assigned teoxoy and unassigned jimblandy May 28, 2025
@cwfitzgerald cwfitzgerald marked this pull request as draft May 28, 2025 15:13
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.

Automatic type conversion not working for select
3 participants