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

Add a FacetValue instruction #4545

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Nov 16, 2024

The new FacetValue instruction represents C as I for some type C and facet type I. It is named FacetValue instead of just Facet to parallel the FacetType instruction.

This PR uses this instruction represent the facet value Self in an impl declaration. This instruction will be used in the future to also support things like:

  • C as I where C is a class; and
  • forming a specific for a generic with a T:! I parameter where T is being given a concrete value.

(Here I is an interface or other non-type facet type.)

Also do some renaming and add some comments to make things a bit more clear.

  • FacetTypeAccess -> FacetAccessType to clarify this is not access of a facet type, but access of the type of a facet
  • .facet_id -> .facet_value_inst_id to parallel the FacetValue instruction

FacetAccessWitness will be in a future PR.

toolchain/check/convert.cpp Outdated Show resolved Hide resolved
toolchain/check/eval.cpp Outdated Show resolved Hide resolved
toolchain/check/eval.cpp Show resolved Hide resolved
toolchain/check/merge.cpp Outdated Show resolved Hide resolved
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: specific @F.1(i32) {}
// CHECK:STDOUT: specific @F.1(@impl.%.loc17) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I think this should be referring to the constant value of the FacetValue instruction, not to the instruction itself, or we won't properly deduplicate specifics for the same type.

Probably just need a constant_values().Get call when we create the FacetValue -- or you can pass the FacetValue directly to TryEvalInst to get a ConstantId without creating an extra instruction.

Copy link
Contributor Author

@josh11b josh11b Nov 21, 2024

Choose a reason for hiding this comment

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

The code is expecting an InstId, not a ConstantId -- is the idea that I will get the InstId for the constant? TryEvalInst also takes an InstId argument, should I pass in invalid or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR with the TryEvalInst approach with an invalid InstId. It does reduce the constant indices back, as well as changing the arguments to these specifics.

toolchain/sem_ir/stringify_type.cpp Outdated Show resolved Hide resolved
Comment on lines 212 to 214
push_inst_id(inst.type_inst_id);
push_string(" as ");
push_inst_id(sem_ir.types().GetInstId(inst.type_id));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be:

Suggested change
push_inst_id(inst.type_inst_id);
push_string(" as ");
push_inst_id(sem_ir.types().GetInstId(inst.type_id));
push_inst_id(sem_ir.types().GetInstId(inst.type_id));
push_string(" as ");
push_inst_id(inst.type_inst_id);

... because we process the pushed parts in reverse order?

Copy link
Contributor Author

@josh11b josh11b Nov 21, 2024

Choose a reason for hiding this comment

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

I applied this change, but it seems this isn't covered by any of our tests. When would this type be stringified?

Copy link
Contributor Author

@josh11b josh11b Nov 21, 2024

Choose a reason for hiding this comment

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

Added the test you suggested, which seemed to have worked! 1371f33

@josh11b
Copy link
Contributor Author

josh11b commented Nov 20, 2024

Should I be concerned about the number of unused constants being generated? The most recent change seems to have increased how much this is happening: d605bf2

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