Skip to content

CFG: Add validator for register allocation. #786

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 0 commits into from
Sep 11, 2022

Conversation

azewierzejew
Copy link
Contributor

@azewierzejew azewierzejew commented Aug 25, 2022

Added validator and unit test for misallocations.

The validator currently has naive implementation in terms of complexity.

It has PR's #774 and #804 included because without them testsuite fails (as validator detects those problems).

@xclerc
Copy link
Contributor

xclerc commented Aug 25, 2022

(I need to give it a proper look, but the failure might
very well be related to #774.)

@azewierzejew
Copy link
Contributor Author

azewierzejew commented Aug 25, 2022

(I need to give it a proper look, but the failure might very well be related to #774.)

Merging that fixed the test failure.

Copy link
Contributor

@xclerc xclerc left a comment

Choose a reason for hiding this comment

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

I have made a first, rather superficial, pass
over everything but the tests. Most, if not
all, comments are minor.

@xclerc xclerc mentioned this pull request Sep 7, 2022
@azewierzejew azewierzejew requested a review from xclerc September 7, 2022 12:36
Copy link
Contributor

@gretay-js gretay-js left a comment

Choose a reason for hiding this comment

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

I read almost the entire PR, it's looks great! I haven't read the printing code and I need to make another pass on the transfer functions.

My main comment is the representation of abstact values in the Domain and the fact the Domain has bits of transfer functions. My view is the domain should be just a set of equations.

The calling convention of Ocaml has a constraint on arguments that are passed in Domainstate.extra_params: they need to be spilled immediately on entry, before any GC or context switch. Is this checked?

It would be nice to make a subdirectory for the validator and split cfg_regalloc_validate.ml into separate files, when structural checks are added.

Copy link
Contributor

@xclerc xclerc left a comment

Choose a reason for hiding this comment

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

The comments are really useful; comments/suggestion are about typos.

Copy link
Contributor

@gretay-js gretay-js left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the test and the printing code. The rest is ready to be merged.

Copy link
Contributor

@xclerc xclerc left a comment

Choose a reason for hiding this comment

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

(Did not read the refactoring of dataflow.)

@azewierzejew azewierzejew requested review from mshinwell and removed request for mshinwell September 9, 2022 15:49
Copy link
Collaborator

@mshinwell mshinwell left a comment

Choose a reason for hiding this comment

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

Approval only for files owned by me

@gretay-js gretay-js merged this pull request into oxcaml:main Sep 11, 2022
mshinwell pushed a commit that referenced this pull request Sep 12, 2022
@azewierzejew azewierzejew deleted the cfg_regalloc_validation branch September 12, 2022 11:21
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.

4 participants