Skip to content

Removal of Targetint.OCaml, move to Target_imm.Imm#500

Merged
mshinwell merged 4 commits intoocaml-flambda:flambda2.0-stablefrom
mshinwell:flambda2-targetint-ocaml
Jun 28, 2021
Merged

Removal of Targetint.OCaml, move to Target_imm.Imm#500
mshinwell merged 4 commits intoocaml-flambda:flambda2.0-stablefrom
mshinwell:flambda2-targetint-ocaml

Conversation

@mshinwell
Copy link
Copy Markdown

The code around Targetint.OCaml is not ideal at present. This PR goes some way towards tidying it up. It moves Targetint.OCaml into the Target_imm module; it is now called Target_imm.Imm. This significantly reduces Targetint-related changes outside the Flambda 2 portion of the tree.

In due course we should squash the Imm module to just have Target_imm, but one step at a time.

@mshinwell
Copy link
Copy Markdown
Author

mshinwell commented Jun 25, 2021

I've realised this needs a check at toplevel that we're not running on 32 bit. I don't think it is worth supporting those platforms here at present (and we already do not in Un_cps anyway). (update: check added)

Copy link
Copy Markdown

@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.

This looks good and should work.
While we're at it, maybe we might want to consider adding some more checks in some of the conversions functions to be more safe and more consistent in the handling of overflows ? For instance One_bit_fewer.of_int silently truncates the input int to make it fit, whereas we probably want to make it produce an error ?

@mshinwell
Copy link
Copy Markdown
Author

CRs added. We also agreed a subsequent patch will rename Target_imm to Targetint_63. (We can subsequently rename to Targetint_31_63 if 32-bit support is needed in the future, though this seems somewhat unlikely.)

@mshinwell
Copy link
Copy Markdown
Author

On second thoughts, the wasm work will need 32-bit support, so let's go for Targetint_31_63 now. The main part of Flambda 2 that doesn't support 32-bit is the primitive translation in Un_cps, but that wouldn't be needed for wasm.

@mshinwell mshinwell merged commit 605c7a4 into ocaml-flambda:flambda2.0-stable Jun 28, 2021
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