Change the way the label for tailrec calls is stored #51
Conversation
…plicitly store the destination label in the `Self` constructor of `Cfg_intf.tail_call_operation`.
backend/asmgen.ml
Outdated
| let fun_body = Cfg_to_linear.run cfg in | ||
| { fd with Linear.fun_body; } | ||
| let fun_body, tailrec_label = Cfg_to_linear.run cfg in | ||
| let fun_tailrec_entry_point_label = Option.value tailrec_label ~default:(-1) in |
There was a problem hiding this comment.
While we are here, it would be better to make Linear.fundecl.fun_tailrec_entry_point_label optional instead of using an "illegal" value and having a dangling pointer. You can probably just take the changes from linear* and emit.mlp files that you have already implemented in 26ae9da.
backend/cfg/cfg.ml
Outdated
| block.terminator.desc | ||
| | Return | Raise _ | Tailcall (Func _) -> block.terminator.desc | ||
| | Tailcall (Self { destination; label_after; }) -> | ||
| Tailcall (Self { destination = f destination; label_after = f label_after; }) |
There was a problem hiding this comment.
Thinking about label_after again, it is not correct to update them here in replace_successor_labels using f, because label_after are not successor labels of Tailcall instructions, and f is intended to be applied only to the successor labels.
It is safe to keep label_after unchanged during CFG transformations.
These labels are not used as block entry labels anywhere in Linear and in CFG. The labels are created during selectgen and not referenced anywhere, until they are printed out in Emit. In any case, they are gone in 4.12 when Spacetime is removed. Until then, if you want to be on the safe side, it would be enough to check somewhere global and abort if spacetime is enabled Config.spacetime when use_ocamlcfg is enabled.
backend/cfg/linear_to_cfg.ml
Outdated
| let rec create_blocks t (i : L.instruction) (block : C.basic_block) | ||
| ~trap_depth ~traps = | ||
| let rec create_blocks (t : t) (i : L.instruction) (block : C.basic_block) | ||
| ~tailrec_label ~trap_depth ~traps = |
There was a problem hiding this comment.
~tailrec_label does not change during construction of the cfg in create_blocks, so it would be better to have tailrec_label as a field of Linear_to_cfg.t.
backend/cfg/cfg_to_linear.ml
Outdated
| | None, Some _ -> tailrec_label := terminator_tailrec_label | ||
| | Some old_trl, Some new_trl -> | ||
| assert (Label.equal old_trl new_trl); | ||
| tailrec_label := terminator_tailrec_label); |
There was a problem hiding this comment.
If tailrec_label and terminator_tailrec_label are already equal, why do we need to update tailrec_label?
| @@ -722,7 +722,9 @@ let emit_instr fallthrough i = | |||
| | Lop(Itailcall_imm { func; label_after; }) -> | |||
| begin | |||
| if func = !function_name then | |||
| I.jmp (label !tailrec_entry_point) | |||
| match !tailrec_entry_point with | |||
There was a problem hiding this comment.
Can you please make this change to other emit.mlp files in "backend" subdirectory? It'd be good to keep them in sync even if it's hard to test them at the moment.
There was a problem hiding this comment.
Indeed, I thought the CI would build the check_all_arches target,
or its dune equivalent.
Remove the `fun_tailrec_entry_point_label` field from `Cfg.t`, and explicitly store the destination label in the `Self` constructor of `Cfg_intf.tail_call_operation`.
Remove the `fun_tailrec_entry_point_label` field from `Cfg.t`, and explicitly store the destination label in the `Self` constructor of `Cfg_intf.tail_call_operation`.
6c5197b8d9 Merge pull request ocaml-flambda#166 from mshinwell/merge-flambda-backend-2023-04-28 0c3dcf956c Fix for ocamldoc 09b9e1cb46 Fix for -zero-alloc-check 71e5e07631 Compilation fixes after merge bf662571ee Merge flambda-backend changes a2556fc75f Add `[%exclave]` support (ocaml-flambda#51) ebe9576b0a Add data race freedom proposal (ocaml-flambda#161) 3f3fc49e0d Merge pull request ocaml-flambda#159 from riaqn/merge-backend 6c635dc542 minor changes after merge 99a0d85320 Merge flambda-backend changes 264246310a Include the modes of values in debugging information (ocaml-flambda#153) 4ecc8a4f17 Remove i386 CI check (ocaml-flambda#155) git-subtree-dir: ocaml git-subtree-split: 6c5197b8d9812079036a83fd74ac5ea752735c6e
bba15422dbf Accept changed test, fix dune file 2f0a6b48399 Layouts version 1 6c5197b8d98 Merge pull request ocaml-flambda#166 from mshinwell/merge-flambda-backend-2023-04-28 0c3dcf956c6 Fix for ocamldoc 09b9e1cb461 Fix for -zero-alloc-check 71e5e076313 Compilation fixes after merge bf662571ee3 Merge flambda-backend changes a2556fc75f4 Add `[%exclave]` support (ocaml-flambda#51) ebe9576b0a2 Add data race freedom proposal (ocaml-flambda#161) 3f3fc49e0d4 Merge pull request ocaml-flambda#159 from riaqn/merge-backend 6c635dc5421 minor changes after merge 99a0d853204 Merge flambda-backend changes 264246310af Include the modes of values in debugging information (ocaml-flambda#153) 4ecc8a4f17a Remove i386 CI check (ocaml-flambda#155) git-subtree-dir: ocaml git-subtree-split: bba15422dbf736511e37db6ea3e952905ff406ed
REVERT: 6c5197b8d9 Merge pull request ocaml-flambda#166 from mshinwell/merge-flambda-backend-2023-04-28 REVERT: 0c3dcf956c Fix for ocamldoc REVERT: 09b9e1cb46 Fix for -zero-alloc-check REVERT: 71e5e07631 Compilation fixes after merge REVERT: bf662571ee Merge flambda-backend changes REVERT: a2556fc75f Add `[%exclave]` support (ocaml-flambda#51) REVERT: ebe9576b0a Add data race freedom proposal (ocaml-flambda#161) REVERT: 3f3fc49e0d Merge pull request ocaml-flambda#159 from riaqn/merge-backend REVERT: 6c635dc542 minor changes after merge REVERT: 99a0d85320 Merge flambda-backend changes REVERT: 264246310a Include the modes of values in debugging information (ocaml-flambda#153) REVERT: 4ecc8a4f17 Remove i386 CI check (ocaml-flambda#155) git-subtree-dir: ocaml git-subtree-split: a7d005a6e537d1d70e60379c05b5e0ee33a294c5
e3076d2e73 Unboxed types v1 (ocaml-flambda#139) e68c72dea2 update HACKING.jst.adoc (ocaml-flambda#165) 6c5197b8d9 Merge pull request ocaml-flambda#166 from mshinwell/merge-flambda-backend-2023-04-28 0c3dcf956c Fix for ocamldoc 09b9e1cb46 Fix for -zero-alloc-check 71e5e07631 Compilation fixes after merge bf662571ee Merge flambda-backend changes a2556fc75f Add `[%exclave]` support (ocaml-flambda#51) ebe9576b0a Add data race freedom proposal (ocaml-flambda#161) 3f3fc49e0d Merge pull request ocaml-flambda#159 from riaqn/merge-backend 6c635dc542 minor changes after merge 99a0d85320 Merge flambda-backend changes 264246310a Include the modes of values in debugging information (ocaml-flambda#153) 4ecc8a4f17 Remove i386 CI check (ocaml-flambda#155) git-subtree-dir: ocaml git-subtree-split: e3076d2e7321a8e8ff18e560ed7a55d6ff0ebf04
This (draft) pull request is in some sense an alternative to #48.
It removes the
fun_tailrec_entry_point_labelfield fromCfg.t,and explicitly stores the destination label in the
Selfconstructor ofCfg_intf.tail_call_operation. By doing so, we avoid the pitfallsof bookkeeping.
A complementary (larger-scale) change would be to also try and
remove the same field from
Linear.fundecl, but it is unclear tome whether it is worthwhile at this stage.