[PATCH] D136537: [llvm-ocaml] Assume pointers are at least 2-bit aligned

Tue Feb 14 11:43:03 PST 2023

alan added inline comments.

================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:250-252
+  CAMLparam1(Value);
   CAMLreturn(to_val(LLVMCreateEnumAttribute(Context_val(C), Int_val(Kind),
                                             Int64_val(Value))));
----------------
jberdine wrote:
> CAMLparam and CAMLreturn do not need to be added (by the composition of this diff and its parent) here, right? There are other functions that seem to be in a similar situation, but asking about one example to check my understanding.
In this diff, I decided to take some liberties by not registering a value with CAMLparam if it is known to not be a GC'd block (for example, if it is an int, or a pointer from LLVM that is represented by setting the low bit). The original bindings on main take this type of liberty a lot by omitting CAMlparam entirely in many places. In my parent diff, I added the CAMlparams and registered all values with CAMlparam, including ones that the GC doesn't need to track, to be safe, but here, I decided to relax which values I registered.

Is this a good choice? Registering all values probably makes the code clearer and reduces room for error (e.g. forgetting to register something), choosing not to register things like ints and 2-bit aligned LLVM pointers with the low bit set probably slightly increases performance.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136537