[PATCH] D65138: [OCaml] Add LLVMInternalizePredicateBindings

Wed Jul 24 02:03:00 PDT 2019

kren1 marked an inline comment as done.
kren1 added inline comments.

================
Comment at: bindings/ocaml/transforms/ipo/llvm_ipo.mli:76
+    If predicate returns [true], that symbol is preserved.
+    NOT THREAD SAFE! *)
+val add_internalize_predicate
----------------
whitequark wrote:
> kren1 wrote:
> > whitequark wrote:
> > > kren1 wrote:
> > > > whitequark wrote:
> > > > > I don't think we make any claims of thread safety for any other API. Why a warning on this one?
> > > > I felt it's necessary to warn the user that the predicate is not passed by value, but its reference is stored in a global variable. This kind of seemed like the most concise way of saying that. Would you prefer it rephrased to more closely match the above or just removed?
> > > Wait. That's actually worse, because it's not *non-thread-safe*, it's not *reentrant*. Which probably doesn't matter for this specific function, but would be good to solve in general.
> > > 
> > > I haven't written any really complex OCaml/C interop code in a few years, so my skills here are a bit rusty... could you please look into a better way to do this? If not, no worries, I'll refresh my memory and dig into it myself, but it might take some weeks to get around to this.
> > I think I addressed this now. Cleaning up is a bit ugly, because I think the lifetime of the predicate closure should basically match the lifetime of the PassManager. I found no examples where this would be handled elsewhere. So I implemented a global association of PassManager and Predicate closure. When you dispose of the pass manager you walk the association and free all predicated associated with the pass manager being disposed. 
> > 
> > Another option would be to extend the PassManager API and make it know about the closures associated with it, but I think this binding has little to do with the PassManager and should stay in its own directory.
> Thank you. Correct me if I'm wrong, but it looks like you could use OCaml APIs to maintain a linked list of predicates, and doing so would simplify the memory management here quite a bit, as well as make it more obviously sound.
You mean use `caml_alloc` and `Store_field` to construct an OCaml linked list in C? I could do that, but not sure it would be cleaner and more obviously sound. I find it easier to reason about C than what OCaml GC will be doing. 

Another approach could be do to some mix-and-match of OCaml and C, for example deal with allocating and traversing the list in OCaml and then have C only allocate the contents of each list. That might work, but it would be spread over atleast 5 files and would require the `Llvm_ipo` to have a public `ref` exposed in its api (so that the PassManager can access it).

There is a similar problem in the first approach but there I could have an `extern value*` (similar to how I do it now).

Another way would be to find an implementation of linked-list in OCaml/LLVM C-api and then use that. That would be my preferred approach, but I couldn't find anything useable there (OCaml seems very GC orientated and LLVM has nothing). Do you have any pointers?

Let me know what would be the best way to proceed.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65138