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 value<->native_pointer intrinsics #855

Merged
merged 15 commits into from
Oct 10, 2022
Merged

Conversation

TheNumbat
Copy link
Contributor

@TheNumbat TheNumbat commented Sep 29, 2022

Implements builtins caml_native_pointer_of_value and caml_native_pointer_to_value.

I've confirmed this passes the tests in my version of the intrinsics library.

@gretay-js gretay-js changed the title Add value<->native_pointer builtins Add value<->native_pointer intrinsics Sep 30, 2022
backend/cmm_helpers.ml Outdated Show resolved Hide resolved
xclerc
xclerc previously approved these changes Sep 30, 2022
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 think we may need to update Selectgen.oper_result_type

@xclerc
Copy link
Contributor

xclerc commented Sep 30, 2022

Wouldn't the typing come from the external?

@gretay-js
Copy link
Contributor

I think we may need to update Selectgen.oper_result_type

no, that's wrong.

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.

@TheNumbat you were right, we do need some conversion operation in Cmm.transl_builtin because the type changes between int and val need to be recorded (to correctly hanlde them in the frametable). We should add Cmm.valuofint and Cmm.intofval (similar to Cmm.intoffloat and inttofloat) and the important case to implement is in Selectgen.oper_result_type.

@TheNumbat TheNumbat marked this pull request as draft September 30, 2022 17:50
@TheNumbat
Copy link
Contributor Author

TheNumbat commented Sep 30, 2022

I added valueofint and intofvalue throughout the backend, and confirmed it still works with my updated intrinsics. They're mostly handled like intoffloat and floatofint, but are treated like opaque_identity for the purposes of register allocation and code emission.

I don't have a test case checking that this fixes an issue with frametable handling, though, so not entirely sure if this is what we want yet. Can someone suggest how to test that?

Also, the upstream build CI check fails now - how should I apply these changes to ocaml/?

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.

Looks good, I just have a few comments.

  1. Please make the PR "ready for review" (not draft).
  2. fixing 32-bit builds: the new operations should only be added in backend subdirectory, don't don't change anything in ocaml/asmcomp
  3. adjust testsuite/tool/parsecmm.mly and lexcmm.mll (it is probably not necessary for the tests in testsuite/tests/asmgen/ to pass because they don't use the new primitives yet, but adding a little test there would be nice).
  4. we don't need to add clambda primitives, there should be no changes in middle_end subdirectory. The only way the new operations can be created is via the new intrinsic. So, no need to change cmmgen.ml either (only cmm_helpers.ml).
  5. update backend/CSEgen similarly to floatofint and intoffloat (it's missing probably because warning 8 exhaustive match isn't on in that file).
  6. No need to expose the new functions in cmm_helpers.mli, they are not used outside of cmm_helpers.ml

backend/selectgen.ml Outdated Show resolved Hide resolved
@TheNumbat TheNumbat marked this pull request as ready for review October 3, 2022 13:45
@mshinwell
Copy link
Collaborator

Hmm I don't understand the middle_end/ changes in this PR; are they needed?

@gretay-js
Copy link
Contributor

Hmm I don't understand the middle_end/ changes in this PR; are they needed?

I don't think so, I also have a comment about it.

@TheNumbat
Copy link
Contributor Author

I've addressed the comments, though I believe backend/CSEgen.ml was already updated (?).

@gretay-js
Copy link
Contributor

I've addressed the comments, though I believe backend/CSEgen.ml was already updated (?).

yes, I somehow missed it, sorry.

@xclerc xclerc dismissed their stale review October 4, 2022 08:41

(large update)

backend/cfg/cfg.ml Outdated Show resolved Hide resolved
@xclerc
Copy link
Contributor

xclerc commented Oct 4, 2022

I don't have a test case checking that this fixes an issue with frametable handling, though, so not entirely sure if this is what we want yet. Can someone suggest how to test that?

I don't have a good way to test that, and it is a bit worrying
indeed. Looking at the contents Xyz__frametable is both
not user-friendly and brittle. For this particular PR, I wonder
whether is would be enough to emit the Reg.typ field when
dumping for instance -dlinear.

@gretay-js
Copy link
Contributor

I don't have a good way to test that, and it is a bit worrying indeed. Looking at the contents Xyz__frametable is both not user-friendly and brittle. For this particular PR, I wonder whether is would be enough to emit the Reg.typ field when dumping for instance -dlinear.

We can already see from -dlinear if a live set before a call has "val" or not (the "val"s are starred). For now, I think we should at least add the test in my comment above (which was resolveD) to the tests in ocaml_intrinsics library.

@xclerc
Copy link
Contributor

xclerc commented Oct 4, 2022

I am not sure that appears everywhere,
for instance when spilling
(misunderstood
was you wrote), but indeed that might be
enough for now.

Although, fundamentally, having a way
to output in a user-friendly fashion the
frame table of a function would probably
be useful. Possibly even for #797.

@xclerc
Copy link
Contributor

xclerc commented Oct 4, 2022

(to be clear, I am not saying that
should happen in this very PR.)

backend/mach.ml Outdated Show resolved Hide resolved
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 purity-related changes should happen
in this PR, but the rest can be delayed.

backend/amd64/emit.mlp Outdated Show resolved Hide resolved
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.

The current version looks good to me.

backend/arm64/emit.mlp Outdated Show resolved Hide resolved
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 arm64 concern should be addressed
(or proved wrong); the rest looks good.

@gretay-js
Copy link
Contributor

The arm64 concern should be addressed (or proved wrong); the rest looks good.

@xclerc this has been addressed

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 code duplication introduced by the last commit
is a bit unfortunate, but can be addressed in another
pull request.

@gretay-js gretay-js mentioned this pull request Oct 10, 2022
@gretay-js gretay-js merged commit ec251fd into ocaml-flambda:main Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants