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

Introduce AnyRawId as a polymorphic field type in typed instructions #4606

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

danakj
Copy link
Contributor

@danakj danakj commented Nov 29, 2024

The Any[...] instruction group structs have a field layout that matches
the specific typed instructions that it groups together. However those
specific instructions may have different field types, or different
numbers of fields, making it impossible for the Any[...] instruction
group to match them all at once.

In this case, it can use the AnyRawId field type to represent that the
specific typed instructions have different field types in that position,
or may not have a field at all.

Previously we used int32_t as this polymorphic placeholder type, which
was implicit and worked somewhat magically:

  • It was not listed in IdKind
  • It was was not implemented explicitly for either FromRaw or ToRaw

However it happened to work because:

  • FromRaw was implemented for every T, and would build as long as T
    was constructible from the raw id value, which is int32_t.
  • In As for converting a specific instruction to a
    group instruction: For any given id field type T in the specific type
    struct, it would be converted to its raw (int32_t) value, then the
    AnyGroupInstruction field would be constructed with FromRaw<int32_t>
    since the polymorphic field type was int32_t.
  • Of course int32_t is constructible from int32_t.
  • And if the specific instruction did not have a field in the matching
    position, the AnyGroupInstruction's field would be default
    constructed, and int32_t is default constructible.

This allowed As to construct the AnyGroupInstruction type
from each of its specific instruction types regardless of what field
types they had.

We can make this more explicit by using a type other than int32_t. We
introduce the AnyRawId specifically for the purpose of being a
polymorphic field type in Any[...] instruction groups.

The AnyRawId type is part of IdKind, removing the need for a special
case in the documentation and rules about field types. And this allows
us to require that T is in IdKind for FromRaw.

  • This also pointed out that FromRaw and ToRaw do not need to be
    implemented for BuiltinTypeKind anymore, as this is no longer a field
    type in any typed instructions, further simplifying the rules to not
    need any exceptions for what is a valid field type.

The AnyRawId type participates in FromRaw by being a member of IdKind
and being constructible from int32_t.

The AnyRawId type is default constuctible so that when the specific
typed instruction has no field in the matching position, it will be
default-constructed with the InstId::InvalidIndex value.

The AnyRawId type does not participate in ToRaw, and this is
documented on the type. This is because conversion from specific typed
instruction to an instruction group is lossy (due to the polymorphism
which requires the AnyRawId type). As such conversion from the
instruction group to a specific instruction is not possible, and thus
the Any[...] instruction group does not need to support being converted
to raw id values.

This is rebased on #4604.

@danakj
Copy link
Contributor Author

danakj commented Nov 29, 2024

cc: @jonmeow as this relates to ongoing work around builtin types
cc: @geoffromer as this relates to ongoing discussion about making the relationships between typed instruction fields more clear

@danakj
Copy link
Contributor Author

danakj commented Nov 29, 2024

Please only look at the commits after and including 0496895 (the one with a description matching the PR), the other commits are from another PR on which this one is rebased (to remove a comment TODO introduced there).

@danakj
Copy link
Contributor Author

danakj commented Nov 29, 2024

#naming I wonder if this should be just called AnyId instead of AnyRawId. The raw part mostly represents my learning path to how I got here, but maybe it's useful there idk.

@danakj
Copy link
Contributor Author

danakj commented Dec 2, 2024

#naming I wonder if this should be just called AnyId instead of AnyRawId. The raw part mostly represents my learning path to how I got here, but maybe it's useful there idk.

Thinking about this over the weekend I think I do like the Raw in there, as it represents that this is not any "Any" like llvm::Any which also knows what type it holds, and that it does not inherit from IdBase since it is only a placeholder for a raw id.

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Thanks, I think this makes a lot of sense.

toolchain/sem_ir/ids.h Outdated Show resolved Hide resolved
toolchain/sem_ir/ids.h Outdated Show resolved Hide resolved
@danakj
Copy link
Contributor Author

danakj commented Dec 3, 2024

Will wait for #4604 to be reviewed and merged before merging this.

toolchain/sem_ir/id_kind.h Show resolved Hide resolved
toolchain/sem_ir/ids.h Outdated Show resolved Hide resolved
The Any[...] instruction group structs have a field layout that matches
the specific typed instructions that it groups together. However those
specific instructions may have different field types, or different
numbers of fields, making it impossible for the Any[...] instruction
group to match them all at once.

In this case, it can use the AnyRawId field type to represent that the
specific typed instructions have different field types in that position,
or may not have a field at all.

Previously we used `int32_t` as this polymorphic placeholder type, which
was implicit and worked somewhat magically:
 - It was not listed in IdKind
 - It was was not implemented explicitly for either FromRaw or ToRaw

However it happened to work because:
 - FromRaw<T> was implemented for every T, and would build as long as T
   was constructible from the raw id value, which is int32_t.
 - In As<AnyGroupInstruction> for converting a specific instruction to a
   group instruction: For any given id field type T in the specific type
   struct, it would be converted to its raw (int32_t) value, then the
   AnyGroupInstruction field would be constructed with FromRaw<int32_t>
   since the polymorphic field type was int32_t.
 - Of course int32_t is constructible from int32_t.
 - And if the specific instruction did not have a field in the matching
   position, the AnyGroupInstruction's field would be default
   constructed, and int32_t is default constructible.

This allowed As<AnyGroupInstruction> to construct the AnyGroupInstruction type
from each of its specific instruction types regardless of what field
types they had.

We can make this more explicit by using a type other than int32_t. We
introduce the AnyRawId specifically for the purpose of being a
polymorphic field type in Any[...] instruction groups.

The AnyRawId type _is_ part of IdKind, removing the need for a special
case in the documentation and rules about field types. And this allows
us to `require` that T is in IdKind for FromRaw<T>.
 - This also pointed out that FromRaw and ToRaw do not need to be
   implemented for BuiltinTypeKind anymore, as this is no longer a field
   type in any typed instructions, further simplifying the rules to not
   need any exceptions for what is a valid field type.

The AnyRawId type participates in FromRaw by being a member of IdKind
and being constructible from int32_t.

The AnyRawId type is default constuctible so that when the specific
typed instruction has no field in the matching position, it will be
default-constructed with the InstId::InvalidIndex value.

The AnyRawId type participates in ToRaw, though this is currently
unused.

This is rebased on carbon-language#4604.
Co-authored-by: Richard Smith <[email protected]>
@danakj danakj enabled auto-merge December 3, 2024 20:32
@danakj danakj added this pull request to the merge queue Dec 3, 2024
Merged via the queue into carbon-language:trunk with commit 7005f39 Dec 3, 2024
8 checks passed
@danakj danakj deleted the any-raw-id branch December 3, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants