Skip to content

Refactor primitive handling in Lambda_to_flambda#1382

Closed
mshinwell wants to merge 1 commit intooxcaml:mainfrom
mshinwell:flambda2-refactor-prim-handling
Closed

Refactor primitive handling in Lambda_to_flambda#1382
mshinwell wants to merge 1 commit intooxcaml:mainfrom
mshinwell:flambda2-refactor-prim-handling

Conversation

@mshinwell
Copy link
Copy Markdown
Collaborator

This allows us to remove the primitive_result_kind function together with some duplicate code (calling transform_primitive).

@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label May 11, 2023
@mshinwell mshinwell requested a review from lthls as a code owner May 11, 2023 15:43
Copy link
Copy Markdown
Contributor

@Ekdohibs Ekdohibs 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, I wonder what the effect of use-visible lets is however. I think we might need to modify close_raise to avoid the duplicated constants (see comments).

let close_raise acc env ~raise_kind ~arg loc exn_continuation =
let acc, exn_cont = close_exn_continuation acc env exn_continuation in
let exn_handler = Exn_continuation.exn_handler exn_cont in
let acc, arg = find_simple acc env arg in
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we already do let acc, args = find_simples acc env args in close_primitive, I think this might duplicate the definition of the constant corresponding to the argument if it is one.

in
let raise_kind = Some (Trap_action.Raise_kind.from_lambda raise_kind) in
let trap_action = Trap_action.Pop { exn_handler; raise_kind } in
let dbg = Debuginfo.from_location loc in
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any reason to do that rather than give dbg as an argument (which is already known in one of the use sites) ?

| Punbox_float -> Punboxed_float
| Pccall _p ->
(* CR ncourant: use native_repr *)
| Pccall { prim_native_repr_res = _, Untagged_int; _} ->layout_int
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
| Pccall { prim_native_repr_res = _, Untagged_int; _} ->layout_int
| Pccall { prim_native_repr_res = _, Untagged_int; _} -> layout_int

| Paddfloat _ | Psubfloat _ | Pmulfloat _ | Pdivfloat _
| Pbox_float _ -> layout_float
| Punbox_float -> Punboxed_float
| Pccall _p ->
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we should add a case Pccall _ when not !Clflags.native_code -> layout_any_value ?

@mshinwell
Copy link
Copy Markdown
Collaborator Author

This is now in #1465 and the comments have been addressed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flambda2 Prerequisite for, or part of, flambda2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants