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

Ensure that each distinct type occurs only once in Module::types #1419

Merged
merged 5 commits into from
Sep 27, 2021

Conversation

jimblandy
Copy link
Member

Each commit should be CI-safe. I recommend reviewing commits separately.

  • Use UniqueArena for types.

    Ensure that each distinct type occurs only once in Module::types, so that we
    can use Eq on Type or TypeInner for type equivalence, without being
    confused by differing Handle<Type> values that point to identical types.

    This removes a number of duplicate types from the ir snapshots.

  • Simplify interpolation defaulting.

    Replace Module::apply_common_default_interpolation with a simpler function
    that handles a single Binding at a time. In exchange for the simplicity, the
    function must be called at each point function arguments, function results, and
    struct members are prepared. (Any missed spots will be caught by the verifier.)

    This approach no longer requires mutating types in the arena, a prerequisite for
    properly handling type identity.

    Applying defaults to struct members when the struct declaration is parsed does
    have a disadvantage, compared to the old whole-module pass: at struct parse
    time, we don't yet know which pipeline stages the struct will be used in. The
    best we can do is apply defaults to anything with a Location binding. This
    causes needless qualifiers to appear in some output. However, it seems that our
    back end languages all tolerate such qualifiers.

  • Consolidate Handle construction code.

  • [glsl-in]: Register new types as necessary during constant solving.

  • Rename UniqueArena methods to more closely resemble HashSet.

    UniqueArena::fetch_or_append becomes insert.

    UniqueArena::try_get becomes get_handle, by analogy with get, that takes a
    type as a key.

src/front/spv/function.rs Outdated Show resolved Hide resolved
src/proc/mod.rs Outdated Show resolved Hide resolved
@kvark
Copy link
Member

kvark commented Sep 27, 2021

That's wonderful, I was looking forward to this change! And well executed, too 👍 🎖️ . Just one note to address.

Replace `Module::apply_common_default_interpolation` with a simpler function
that handles a single `Binding` at a time. In exchange for the simplicity, the
function must be called at each point function arguments, function results, and
struct members are prepared. (Any missed spots will be caught by the verifier.)

This approach no longer requires mutating types in the arena, a prerequisite for
properly handling type identity.

Applying defaults to struct members when the struct declaration is parsed does
have a disadvantage, compared to the old whole-module pass: at struct parse
time, we don't yet know which pipeline stages the struct will be used in. The
best we can do is apply defaults to anything with a `Location` binding. This
causes needless qualifiers to appear in some output. However, it seems that our
back end languages all tolerate such qualifiers.
Ensure that each distinct type occurs only once in `Module::types`, so that we
can use `Eq` on `Type` or `TypeInner` for type equivalence, without being
confused by differing `Handle<Type>` values that point to identical types.

This removes a number of duplicate types from the ir snapshots.

Fixes gfx-rs#1385.
`UniqueArena::fetch_or_append` becomes `insert`.

`UniqueArena::try_get` becomes `get_handle`, by analogy with `get`, that takes a
type as a key.
@jimblandy
Copy link
Member Author

That last forced push just adds a "fixes #1385" note to the commit message. No code change.

@kvark kvark merged commit 6e4401a into gfx-rs:master Sep 27, 2021
@jimblandy jimblandy deleted the unique-arena branch September 28, 2021 17:03
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