Skip to content
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

[wgsl-in] Refactor construction lowering code. #2577

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Oct 21, 2023

(Draft because it depends an a bunch of other stuff.)

  • [wgsl-in] Unify ConcreteConstructor and ConcreteConstructorHandle.

    Replace the ConcreteConstructor and ConcreteConstructorHandle
    types in front::wgsl::lower::construction with a single type
    Constructor with a type parameter that determines how it refers to
    Naga types.

    In the single-argument vector construction case, construct Compose
    expressions when the lengths don't match, and leave the problem for
    validation to catch; validate_compose is a better place to invest
    time in improving diagnostics (and arguably producess more specific
    error messages already).

  • [wgsl-in] Remove Components enum in favor of slice patterns.

    Remove front::wgsl::lower::construction::Components, and simply use
    slice patterns to decide which Naga code to generate. Supply the type
    of the first argument, if available, as an Option. This is slightly
    redundant, as the Option is Some if and only if the slice is
    non-empty, but in practice it is not too messy, and we can add match
    arms to check for misalignment.

@jimblandy jimblandy changed the title [wgsl-in] Remove Components enum in favor of slice patterns. [wgsl-in] Refactor construction lowering code. Oct 22, 2023
@jimblandy jimblandy force-pushed the construction-cleanup-components branch from 8b30632 to b6b0b68 Compare October 23, 2023 19:36
@jimblandy jimblandy marked this pull request as ready for review October 23, 2023 19:37
@jimblandy jimblandy requested a review from a team as a code owner October 23, 2023 19:37
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

  • [wgsl-in] Unify ConcreteConstructor and ConcreteConstructorHandle.
    Replace the ConcreteConstructor and ConcreteConstructorHandle
    types in front::wgsl::lower::construction with a single type
    Constructor with a type parameter that determines how it refers to
    Naga types.

Nice refactor!

In the single-argument vector construction case, construct Compose
expressions when the lengths don't match, and leave the problem for
validation to catch; validate_compose is a better place to invest
time in improving diagnostics (and arguably producess more specific
error messages already).

This used to error with:

error: cannot cast a vec3<f32> to a vec2<f32>
   ┌─ wgsl:91:26
   │
91 │     let test = vec2<f32>(vec3<f32>());
   │                          ^^^^^^^^^^ cannot cast a vec3<f32> to a vec2<f32>

It now errors with:

Naga module validation failed on test 'access.wgsl': WithSpan { inner: Function { handle: [2], name: "test_matrix_within_array_within_struct_accesses", source: Expression { handle: [71], source: Compose(ComponentCount { given: 3, expected: 2 }) } }, spans: [(Span { start: 1431, end: 2264 }, "naga::Function [2]"), (Span { start: 1921, end: 1943 }, "naga::Expression [71]")] }

I think we should keep this as it was (at least for now) as there are other cases where you'd run into this same error and it would keep things consistent.

  • [wgsl-in] Remove Components enum in favor of slice patterns.
    Remove front::wgsl::lower::construction::Components, and simply use
    slice patterns to decide which Naga code to generate. Supply the type
    of the first argument, if available, as an Option. This is slightly
    redundant, as the Option is Some if and only if the slice is
    non-empty, but in practice it is not too messy, and we can add match
    arms to check for misalignment.

The nice thing about the Components enum was that it worked around rust's limitation on matching on Vecs. With this change, we are matching on slices and then have to copy the components.

I also found it easier to read the code before (even if it was a bit more lengthy) since there are now 3 things we are matching on inside a tuple + the components being a tuple themselves (containing the handle and the span).

@jimblandy
Copy link
Member Author

The nice thing about the Components enum was that it worked around rust's limitation on matching on Vecs. With this change, we are matching on slices and then have to copy the components.

I also found it easier to read the code before (even if it was a bit more lengthy) since there are now 3 things we are matching on inside a tuple + the components being a tuple themselves (containing the handle and the span).

Okay - the intent was to make things clearer. I'll try putting it back. There was a reason I introduced the vector copies, related to my abstract types plan, but I'll see if I can keep them out. I do feel like there was room for improvement in the original Components, but maybe I didn't find the right balance.

Replace the `ConcreteConstructor` and `ConcreteConstructorHandle`
types in `front::wgsl::lower::construction` with a single type
`Constructor` with a type parameter that determines how it refers to
Naga types.
@jimblandy jimblandy force-pushed the construction-cleanup-components branch from b6b0b68 to 787b9bf Compare October 24, 2023 20:40
@jimblandy
Copy link
Member Author

I ended up removing pretty much any changes to Components, so all that's left here is the stuff that I think you liked.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@teoxoy teoxoy merged commit ada3cd8 into gfx-rs:master Oct 24, 2023
5 checks passed
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.

2 participants