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

[Josh Berdine via Phabricator via llvm-commits]

jberdine requested changes to this revision.
jberdine added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: StephenFan.

Just a few initial comments so far, I have read and tested this some, but need to do more. Also, I think it would be easier to review if this diff and D136400 <https://reviews.llvm.org/D136400> were combined into a single diff.



================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:38-40
+value to_val(void *ptr) { return ((value)ptr) + 1; }
 
+void *from_val(value v) { return (void *)(v - 1); }
----------------
It may not matter, but I would be more comfortable to use bitwise operations to set and clear the low bit of pointers. And I think it would be better to add an assertion to test that the low bit is clear, as a way to more explicitly document the assumption, and to give us a chance to catch it in a debug build if it is ever violated.


================
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))));
----------------
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.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:602
   unsigned Length = LLVMCountStructElementTypes(Type_val(StructTy));
   LLVMTypeRef *Temp = alloc_temp(sizeof(LLVMTypeRef) * Length);
   LLVMGetStructElementTypes(Type_val(StructTy), Temp);
----------------
`alloc_temp` expects an ocaml block, not a size


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1683
   LLVMSetThreadLocal(Value_val(GlobalVar), Bool_val(IsThreadLocal));
-  CAMLreturn(Val_unit);
+  Val_unit;
 }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136537