-
Notifications
You must be signed in to change notification settings - Fork 89
Change the way the label for tailrec calls is stored #51
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
Conversation
…plicitly store the destination label in the `Self` constructor of `Cfg_intf.tail_call_operation`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This (draft) pull request is in some sense an alternative to #48.
It removes the
fun_tailrec_entry_point_label
field fromCfg.t
,and explicitly stores the destination label in the
Self
constructor 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.