Skip to content

Fix some ICEs with bools #826

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

Merged
merged 2 commits into from
Dec 15, 2021
Merged

Fix some ICEs with bools #826

merged 2 commits into from
Dec 15, 2021

Conversation

khyperia
Copy link
Contributor

#809 missed some cases where to/from_immediate was inlined into local code, causing some ICEs

@khyperia khyperia requested a review from eddyb as a code owner December 15, 2021 08:58
Comment on lines 932 to 934
// WARN! This should go through to_immediate, but because we only have a Scalar,
// not a Ty, we can't call to_immediate here (even though we implement it as a
// no-op), so "inline" the no-op here and do nothing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that why to_immediate_scalar exists, and to_immediate merely proxies to it? rustc_codegen_ssa calls to_immediate_scalar in a few places where to_immediate isn't available (e.g. for each half of a ScalarPair).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahah, yes, I totally remembered to_immediate_scalar existed, mhmm, definitely. I'm going to go with the excuse that "I don't think it existed when I originally wrote this code, and I just copied the comment/behavior forward with this new change" and hope that's right ✨

@khyperia khyperia requested a review from eddyb December 15, 2021 13:43
@khyperia khyperia enabled auto-merge (squash) December 15, 2021 13:52
@khyperia khyperia merged commit f08819f into main Dec 15, 2021
@khyperia khyperia deleted the icy-bools branch December 15, 2021 14:10
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