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 support for deduction of most kinds of type constant. #4389

Merged
merged 5 commits into from
Oct 9, 2024

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Oct 8, 2024

This adds deduction in all the cases where we can match the instruction
fields of the parameter against the corresponding instruction fields of
the argument. This handles all current type constants except for struct
types, for which we would want to match by field name.

@zygoloid zygoloid added the dependent Depends on another issue/PR label Oct 8, 2024
@zygoloid
Copy link
Contributor Author

zygoloid commented Oct 8, 2024

Depends on #4380.

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Basically LGTM, feel free to merge when ready.

toolchain/check/deduce.cpp Outdated Show resolved Hide resolved
Comment on lines +335 to +338
worklist_.AddInstArg(kind0, param_inst.arg0(), arg_inst.arg0(),
needs_substitution);
worklist_.AddInstArg(kind1, param_inst.arg1(), arg_inst.arg1(),
needs_substitution);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth at all conditioning this based on the # of args to the inst? Or if the arg is invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could have different case blocks for different numbers of arguments, but I think it's probably not worth the complexity and tighter coupling. The switch in AddInstArg immediately catches the case of no corresponding argument, so we should deal with that case quickly.

So far, we don't have any cases where the arg can be present but invalid. I guess we can handle that if/when it starts coming up.

zygoloid and others added 4 commits October 8, 2024 22:52
This adds deduction in all the cases where we can match the instruction
fields of the parameter against the corresponding instruction fields of
the argument. This handles all current type constants except for struct
types, for which we would want to match by field name.
Co-authored-by: Jon Ross-Perkins <[email protected]>
@zygoloid zygoloid added this pull request to the merge queue Oct 9, 2024
Merged via the queue into carbon-language:trunk with commit 6410d6e Oct 9, 2024
8 checks passed
@zygoloid zygoloid deleted the toolchain-deduce-more branch October 9, 2024 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependent Depends on another issue/PR toolchain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants