[PATCH] D136400: [llvm-ocaml] Migrate from naked pointers to prepare for OCaml 5

Sun Feb 26 06:58:08 PST 2023

jberdine added a comment.

Adding comments about the reasoning behind e.g. avoiding the write barrier seems like a good idea. I don't know if it would be better to have one comment block near the top of the file, or many comments sprinkled around. There might be a lot of redundancy which would be noisy and easy to get out of sync.

There is some overhead to the CAMLparam, etc. macros. Each one involves some memory operations itself, checks the thread local state of the runtime system, and also increases the number of roots that need to be scanned each time the GC is run. I would be hesitant to add them uniformly without a good benchmark suite to measure the impact and see that it doesn't matter.

================
Comment at: llvm/bindings/ocaml/target/target_ocaml.c:148
+value llvm_datalayout_offset_of_element(value Ty, value Index, value DL) {
+  CAMLparam1(DL);
+  CAMLreturn(caml_copy_int64(
----------------
alan wrote:
> jberdine wrote:
> > alan wrote:
> > > jberdine wrote:
> > > > left over
> > > This root is necessary. `DataLayout.t` is an OCaml custom block, not a pointer with the low bit set. `Int64.t` is also boxed. So, this function allocates on the OCaml heap and the GC needs to know that `DL` is live.
> > I understand that `DL` is a custom block, but its value is only used before the allocation in `caml_copy_int64`. This is the same before and after this diff, or am I overlooking something?
> `DL` is a parameter, so it needs to be live when the function ends. Otherwise, a caller could pass a `DataLayout.t` value, and that value could be invalidated by the GC during the function call.
I'm sorry but I still don't understand. Suppose the GC runs when `caml_copy_int64` allocates, and happens to move the value `DL` points to. Then note that `DL` is no longer needed since it is used only to compute the argument of `LLVMOffsetOfElement` and in particular is dead after the allocation in `caml_copy_int64`. Also, if the caller of `llvm_datalayout_offset_of_element` needs what it passed as the `DL` argument, then it will be live and a root in the caller, and the GC will update the caller's pointer when moving the block pointed to by `DL`. So I don't see the issue, but I am not sure I understand the point you are making. Do you see what I mean?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136400/new/

https://reviews.llvm.org/D136400