[PATCH] D65138: [OCaml] Add LLVMInternalizePredicateBindings

Tue Jul 23 09:18:12 PDT 2019

whitequark 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
----------------
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.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65138