Page MenuHomePhabricator

[LLVM-C][OCaml] Add a fast linker binding
Needs ReviewPublic

Authored by kren1 on Mon, Jul 22, 3:01 AM.

Details

Summary

The current LLVM-C/Ocaml linker API is very slow when linking many files
together. This patch exposes and additional API that does not destroy
the Linker object between each linking call.

Diff Detail

Event Timeline

kren1 created this revision.Mon, Jul 22, 3:01 AM
Herald added a project: Restricted Project. · View Herald Transcript
This revision is now accepted and ready to land.Tue, Jul 23, 4:22 AM

@modocache could you commit this for me?

CodaFi requested changes to this revision.Tue, Jul 23, 9:07 AM
CodaFi added a subscriber: CodaFi.
CodaFi added inline comments.
include/llvm-c/Linker.h
54

Can you document the lifetime of the linked module?

lib/Linker/LinkModules.cpp
617

Can you expose these two defaulted arguments in the bindings as well?

This revision now requires changes to proceed.Tue, Jul 23, 9:07 AM
kren1 updated this revision to Diff 211440.Wed, Jul 24, 1:32 AM
kren1 marked an inline comment as done.

Document lifetime of source, remove unnecessary arguments

kren1 marked an inline comment as done.Wed, Jul 24, 1:35 AM
kren1 added inline comments.
lib/Linker/LinkModules.cpp
617

Exposing them is not trivial, because I would have to introduce new types for Flag and a trampoline for the callback. I removed them to be implicit and in line with LLVMLinkModules2. If you really want me to add them, I would prefer to do it as a separate diff.

CodaFi added inline comments.Wed, Jul 24, 7:29 AM
lib/Linker/LinkModules.cpp
617

I realize it’s non-trivial, but our commitment to ABI compatibility makes revising these public interfaces much more difficult after the fact than getting it right up front.

FWIW, the result of the internalize bindings patch you have apply here as well, and you need only provide a thunk to translate the C binding flags to the few linker flags LLVM currently supports. If you’d like, I can generate that binding and tag you in a review, then you can rebase the OCaml bindings on top.

kren1 updated this revision to Diff 211547.Wed, Jul 24, 10:18 AM

Properly expose the 2 default arguments to linkInModule

kren1 updated this revision to Diff 211685.Thu, Jul 25, 1:17 AM
kren1 marked 2 inline comments as done.

Simplify memory management for the callback value

CodaFi added inline comments.Fri, Jul 26, 6:45 PM
include/llvm-c/Linker.h
35

These names should be qualified as above.

typedef enum {

LLVMLinkerFlagsNone = 0,
LLVMLinkerFlagsOverrideFromSrc = 1,
LLVMLinkerFlagsLinkOnlyNeeded = 2

} LLVMLinkerFlags;

kren1 updated this revision to Diff 212049.Sat, Jul 27, 2:57 AM
kren1 marked an inline comment as done.

Qualify linker flags

whitequark requested changes to this revision.Sat, Jul 27, 3:03 AM
whitequark added inline comments.
bindings/ocaml/linker/linker_ocaml.c
40

I think the proliferation of non-reentrant/non-threadsafe functions in the API is an issue. (Right now OCaml has a global lock, but a multicore runtime is in works.) The approach in D65138 is better, but still seems error-prone to me; I'll need to take a closer look at the OCaml runtime feature to see if perhaps a better one is possible.

This revision now requires changes to proceed.Sat, Jul 27, 3:03 AM
kren1 marked an inline comment as done.Sat, Jul 27, 5:30 AM
kren1 added inline comments.
bindings/ocaml/linker/linker_ocaml.c
40

I can't do the D65138 approach, because the callback doesn't pass a context, but it would be easier in this case because there is no need to worry about life times.

whitequark added inline comments.Sat, Jul 27, 5:35 AM
bindings/ocaml/linker/linker_ocaml.c
40

I would personally consider any C API function that does not take a context a (usability) bug, since the LLVM C API is primarily useful for writing bindings from other languages, most of which have some kind of closure mechanism. I think in this case it is clearly justifiable to add an LLVMLinkInModule2 that fixes this.

kren1 marked an inline comment as done.Sat, Jul 27, 5:44 AM
kren1 added inline comments.
bindings/ocaml/linker/linker_ocaml.c
40

Ah good point, I'Il do that. Thanks

kren1 updated this revision to Diff 212136.Mon, Jul 29, 2:36 AM
kren1 marked 2 inline comments as done.

Pass the closure via context instead of global variable