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

Refactor cmm functions to be more generic over operand width #3404

Merged
merged 7 commits into from
Mar 5, 2025

Conversation

jvanburen
Copy link
Contributor

No description provided.

Copy link

Selection Change Check

This PR modifies the original selection pass (targeting Mach).
Please check whether the changes should also be applied to the
CFG variant of the pass.

@jvanburen
Copy link
Contributor Author

This PR refactors cmm_helpers in preparation for unboxed 8 and 16-bit integers to be added to the compiler.
It replaces many functions that were copied for each width with a single more generic version, and then updates to_cmm to use it.

@jvanburen jvanburen marked this pull request as ready for review December 23, 2024 21:36
@jvanburen jvanburen requested a review from mshinwell December 23, 2024 21:36
@jvanburen jvanburen marked this pull request as draft December 27, 2024 21:06
@jvanburen jvanburen changed the base branch from main to cmm-refactor-unboxed-fields January 2, 2025 19:55
Copy link

github-actions bot commented Jan 2, 2025

Parser Change Checklist

This PR modifies the parser. Please check that the following tests are updated:

  • parsetree/source_jane_street.ml

This test should have examples of every new bit of syntax you are adding. Feel free to just check the box if your PR does not actually change the syntax (because it is refactoring the parser, say).

@jvanburen jvanburen force-pushed the cmm-refactor-unboxed-fields branch from d1acc48 to abcd725 Compare January 3, 2025 14:59
@mshinwell mshinwell added the cmm Cmm language / helpers changes label Jan 15, 2025
@jvanburen jvanburen force-pushed the cmm-refactor-unboxed-fields branch from 16f1b23 to 443a81c Compare January 29, 2025 17:14
@jvanburen jvanburen force-pushed the refactor-cmm-for-more-integer-widths branch from 34a5e6a to 6392eea Compare January 29, 2025 17:53
@jvanburen jvanburen force-pushed the cmm-refactor-unboxed-fields branch from 443a81c to 2f5d60b Compare February 13, 2025 22:45
@jvanburen jvanburen force-pushed the refactor-cmm-for-more-integer-widths branch from 6392eea to e5c7499 Compare February 13, 2025 22:52
Base automatically changed from cmm-refactor-unboxed-fields to main February 18, 2025 16:42
@jvanburen jvanburen marked this pull request as ready for review February 18, 2025 16:43
@jvanburen jvanburen requested review from Gbury and removed request for mshinwell February 18, 2025 16:44
@jvanburen jvanburen force-pushed the refactor-cmm-for-more-integer-widths branch from 338f838 to e0b8691 Compare February 18, 2025 17:29
@jvanburen jvanburen requested a review from TheNumbat February 21, 2025 20:53
Copy link
Contributor

@Gbury Gbury left a comment

Choose a reason for hiding this comment

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

Seems good overall, modulo the few changes which change/allow more things in mixed blocks, and which don't seem to be part of the refactor (and which would need more in-depth review to make sure the code is correct and that the rest of the code dealing with them is correct for these new values).

Copy link
Contributor

@TheNumbat TheNumbat left a comment

Choose a reason for hiding this comment

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

lgtm; some comments on comments

@jvanburen jvanburen merged commit 7811bd7 into main Mar 5, 2025
23 checks passed
@jvanburen jvanburen deleted the refactor-cmm-for-more-integer-widths branch March 5, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmm Cmm language / helpers changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants