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

[Dmitrii Kosarev via Phabricator via llvm-commits]

Kakadu requested changes to this revision.
Kakadu added a comment.
This revision now requires changes to proceed.

I was brought here from https://discuss.ocaml.org/t/proposal-care-more-about-ocaml-bindings-for-popular-libraries/11451 and https://discourse.llvm.org/t/rfc-moving-ocaml-bindings-to-peripheral-tier-and-disabling-them-by-default/68290/10.

I hope my comments will be helpful. There is some weird stuff here, but if author's code started to pass the tests after these changes, we could address other issues with binding in separate PRs (this is already rather long)



================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:45
+                       "treats them as integers");
+  return (void *)(v ^ 1);
 }
----------------
Could you add a comment why these two functions are needed? I always though that out-of-heap values should be wrapped into Abstract_tag. At least the manual says that https://v2.ocaml.org/manual/intfc.html#ss:c-outside-head

Sorry for asking stupid questions, I was teleported there from https://discuss.ocaml.org/t/proposal-care-more-about-ocaml-bindings-for-popular-libraries/11451


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:61
 
 value cstr_to_string(const char *Str, mlsize_t Len) {
   if (!Str)
----------------
Am I understanding right... LLVM uses non-zero-terminated strings under the hood, so this function is need?

In case of zero terminated string it would be better to use caml_copy_string https://github.com/ocaml/ocaml/blob/5.0/runtime/alloc.c#L208


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:118
 
-static value alloc_variant(int tag, void *Value) {
+void *alloc_temp(value Elements) {
+  unsigned Length = Wosize_val(Elements);
----------------
I'm not 100% follow the intent of this function. It looks like it tries to convert OCaml array of OCaml values to C array of OCaml values. But if it is true, I'm not sure that llvm_struct_element_types works as expected.

In I understood intent correctly, it should be renamed to array_of_ocaml_array or something.....


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:663
+  unsigned Length = LLVMCountStructElementTypes(Type_val(StructTy));
+  LLVMTypeRef *Temp = alloc_temp(Length);
+  LLVMGetStructElementTypes(Type_val(StructTy), Temp);
----------------
Could you double check that there is no bug here? See my comment about alloc_temp. 


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:705
+  free(Temp);
+  CAMLreturn(Arr);
 }
----------------
This code looks suspicious. If we are going to return an array, why not to try to use `caml_alloc_array` function? 



================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:940
+  const unsigned *Indices = LLVMGetIndices(Value_val(Instr));
+  Array = caml_alloc_tuple(Length);
+  for (unsigned I = 0; I < Length; I++) {
----------------
Here we again allocate a tuple and return an array. Looks weird.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1405
+  CAMLreturn(to_val(Value));
+}
+
----------------
I'm wondering... Is it a good idea to define a macro for more short definitions of binary operations above?


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:2914
+  return llvm_build_atomicrmw_native(argv[0], argv[1], argv[2], argv[3],
+                                     argv[4], argv[5], argv[6]);
 }
----------------
Isn't that strange that argn is not used here?


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:3126
   for (Tl = Incoming; Tl != Val_int(0); Tl = Field(Tl, 1)) {
     value Hd = Field(Tl, 0);
+    LLVMValueRef V = Value_val(Field(Hd, 0));
----------------
It looks like the 'value' is not required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400