[OCaml][NPM] Add OCaml bindings to new pass manager #66421
Conversation
|
If I do use the result type, I want to share a single allocation of |
| LLVMRunPasses(Module_val(M), String_val(Passes), TargetMachine_val(TM), | ||
| PassBuilderOptions_val(Options)); | ||
| if (Err == LLVMErrorSuccess) { | ||
| value result = caml_alloc(1, 0); |
There was a problem hiding this comment.
Regarding the comment about sharing a single allocation, do you mean this call to caml_alloc? If so, do not worry about it, this is vanishingly cheap (usually just a pointer increment and test) compared to what LLVMRunPasses does.
There was a problem hiding this comment.
Yes, this is what I meant by sharing the allocation.
There was a problem hiding this comment.
This LGTM. I made a few minor comments, but nothing that needs another round of review.
Regarding the question of how to expose the LLVM error reporting mechanics, there are 3 reasonable options I know of:
- use
resultjust as you have - use
optionwhereNonemeans "no error" andSome msgmeans an error described by msg occurred - use exceptions
If you want, you could post on the ocaml discuss with the function signature and a brief description of what underlying functionality is being exposed. I would not be surprised to see different people offer support for all of the options. Overall though, there is a move in the ocaml community away from using exceptions toward either option or result, so I expect choice 3 to be the least supported. Choosing between 1 and 2 is harder. If LLVMRunPasses was very light and expected to be fast, using option 2 would avoid an allocation in the common case, which might be worth it. But in this case that won't matter, and it is probably (my opinion) better to have the no-error case be reported as Ok and the error case be reported as Error rather than None and Some error which is opposite to usual usage where None represents an unspecified error.
| (** [run_passes m passes tm opts] runs a set of passes over a module. The | ||
| format of the string [passes] is the same as opt's -passes argument for | ||
| the new pass manager. Individual passes may be specified, separated by | ||
| commas. Full pipelines may also be invoked. *) |
There was a problem hiding this comment.
Would it be helpful to add a crossref to LLVMRunPasses, or whatever the corresponding C++ function in the doxygen is?
I think it would be good to explicitly state what the two cases of the result mean in LLVM terms, in particular to state that the returned string is obtained from LLVMGetErrorMessage, if only for users who are familiar with LLVM-C.
There was a problem hiding this comment.
This is the definition of LLVMRunPasses, which seems to exist for the C API:
This seems to be the corresponding C++ function: https://llvm.org/doxygen/classllvm_1_1PassManager.html#aef5d9142acafceffd14c76b8ddd0fd4e where ModulePassManager is an alias for PassManager<Module>: https://llvm.org/doxygen/namespacellvm.html#a083c8f57e0e3a1011bf2ea8673700558
Should I just put down the C function LLVMRunPasses? Or should I put down ModulePassManager::run? What would be easiest for readers to understand and look up? I think that the C API function has enough additional functionality that the wrapped C++ function doesn't have that I should refer to the C function if I were to put a crossref. What do you think?
There was a problem hiding this comment.
Yes, agree that referring to LLVMRunPasses makes sense.
| -> llpassbuilder_options | ||
| -> (unit, string) result | ||
|
|
||
| (** Creates a new set of options for a PassBuilder. *) |
| (** Creates a new set of options for a PassBuilder. *) | ||
| val create_passbuilder_options : unit -> llpassbuilder_options | ||
|
|
||
| (** Toggles adding the VerifierPass for the PassBuilder. *) |
| val passbuilder_options_set_verify_each | ||
| : llpassbuilder_options -> bool -> unit | ||
|
|
||
| (** Toggles debug logging. *) |
| : llpassbuilder_options -> int -> unit | ||
|
|
||
| (** Tuning option to disable promotion to scalars in LICM with MemorySSA, if | ||
| the number of access is too large. See |
There was a problem hiding this comment.
| the number of access is too large. See | |
| the number of accesses is too large. See |
| val passbuilder_options_set_inliner_threshold | ||
| : llpassbuilder_options -> int -> unit | ||
|
|
||
| (** Disposes of the options. *) |
There was a problem hiding this comment.
This has no crossref to a C++ function. The C API wrapper function merely calls delete.
|
|
||
| let context = Llvm.global_context () | ||
|
|
||
| let m = Llvm.create_module context "mymodule" |
There was a problem hiding this comment.
It would perhaps be a more effective test if this loaded something like llvm/test/Other/new-pass-manager.ll rather than just using an empty module.
There was a problem hiding this comment.
I couldn't figure out how to manipulate the OCaml test infrastructure to read llvm/test/Other/new-pass-manager.ll , so I recreated it via the OCaml API. After all, the actual LLVM IR in that file is just a small function, and the rest of it is test scripts irrelevant to testing the OCaml bindings.
alan-j-hu
force-pushed
the
ocaml-newpassmanager
branch
from
4fc0fd2
to
eb060a0
Compare
alan-j-hu
force-pushed
the
ocaml-newpassmanager
branch
from
eb060a0
to
e9a782a
Compare
I'm still torn on how to translate the LLVM API's C error handling to OCaml. Should I use the result type or exceptions?