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`.
gretay-js
left a comment
gretay-js
left a comment
There was a problem hiding this comment.
It looks much cleaner with this representation, thank you! I just have a couple of small comments.
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?
| 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`.
6c5197b Merge pull request oxcaml#166 from mshinwell/merge-flambda-backend-2023-04-28 0c3dcf9 Fix for ocamldoc 09b9e1c Fix for -zero-alloc-check 71e5e07 Compilation fixes after merge bf66257 Merge flambda-backend changes a2556fc Add `[%exclave]` support (oxcaml#51) ebe9576 Add data race freedom proposal (oxcaml#161) 3f3fc49 Merge pull request oxcaml#159 from riaqn/merge-backend 6c635dc minor changes after merge 99a0d85 Merge flambda-backend changes 2642463 Include the modes of values in debugging information (oxcaml#153) 4ecc8a4 Remove i386 CI check (oxcaml#155) git-subtree-dir: ocaml git-subtree-split: 6c5197b
bba15422dbf Accept changed test, fix dune file 2f0a6b48399 Layouts version 1 6c5197b Merge pull request oxcaml#166 from mshinwell/merge-flambda-backend-2023-04-28 0c3dcf9 Fix for ocamldoc 09b9e1c Fix for -zero-alloc-check 71e5e07 Compilation fixes after merge bf66257 Merge flambda-backend changes a2556fc Add `[%exclave]` support (oxcaml#51) ebe9576 Add data race freedom proposal (oxcaml#161) 3f3fc49 Merge pull request oxcaml#159 from riaqn/merge-backend 6c635dc minor changes after merge 99a0d85 Merge flambda-backend changes 2642463 Include the modes of values in debugging information (oxcaml#153) 4ecc8a4 Remove i386 CI check (oxcaml#155) git-subtree-dir: ocaml git-subtree-split: bba15422dbf736511e37db6ea3e952905ff406ed
REVERT: 6c5197b Merge pull request oxcaml#166 from mshinwell/merge-flambda-backend-2023-04-28 REVERT: 0c3dcf9 Fix for ocamldoc REVERT: 09b9e1c Fix for -zero-alloc-check REVERT: 71e5e07 Compilation fixes after merge REVERT: bf66257 Merge flambda-backend changes REVERT: a2556fc Add `[%exclave]` support (oxcaml#51) REVERT: ebe9576 Add data race freedom proposal (oxcaml#161) REVERT: 3f3fc49 Merge pull request oxcaml#159 from riaqn/merge-backend REVERT: 6c635dc minor changes after merge REVERT: 99a0d85 Merge flambda-backend changes REVERT: 2642463 Include the modes of values in debugging information (oxcaml#153) REVERT: 4ecc8a4 Remove i386 CI check (oxcaml#155) git-subtree-dir: ocaml git-subtree-split: a7d005a
e3076d2 Unboxed types v1 (oxcaml#139) e68c72d update HACKING.jst.adoc (oxcaml#165) 6c5197b Merge pull request oxcaml#166 from mshinwell/merge-flambda-backend-2023-04-28 0c3dcf9 Fix for ocamldoc 09b9e1c Fix for -zero-alloc-check 71e5e07 Compilation fixes after merge bf66257 Merge flambda-backend changes a2556fc Add `[%exclave]` support (oxcaml#51) ebe9576 Add data race freedom proposal (oxcaml#161) 3f3fc49 Merge pull request oxcaml#159 from riaqn/merge-backend 6c635dc minor changes after merge 99a0d85 Merge flambda-backend changes 2642463 Include the modes of values in debugging information (oxcaml#153) 4ecc8a4 Remove i386 CI check (oxcaml#155) git-subtree-dir: ocaml git-subtree-split: e3076d2
2 participants
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.