Skip to content

Only export symbols explicitly tagged with HALIDE_EXPORT (#4651)#5655

Closed
steven-johnson wants to merge 16 commits intomasterfrom
srj/exporting
Closed

Only export symbols explicitly tagged with HALIDE_EXPORT (#4651)#5655
steven-johnson wants to merge 16 commits intomasterfrom
srj/exporting

Conversation

@steven-johnson
Copy link
Copy Markdown
Contributor

This is a work-in-progress to do selective symbol support from libHalide. It takes the approach of defaulting all visibility to hidden, with only things tagged with HALIDE_EXPORT being exported. It's a work in progress for several reasons:

  • I added symbols by starting with none, then gradually adding until all our tests and apps built and ran properly on Linux. There may well be extra things we need to expose that I missed, or are platform-specific.
  • There are a lot of Internal symbols that we use only for our tests (or by the autoscheduler plugins). For now, I defined two additional macros (HALIDE_EXPORT_FOR_TEST and HALIDE_EXPORT_FOR_PLUGINS) that are functionally identical to HALIDE_EXPORT, mainly for my own development purposes. I suspect that replacing both with something like HALIDE_INTERNAL_API might make sense. Thoughts? (In the long run, I'd like to minimize the stuff we need to export for either of these.)
  • There's also some thought that we should pre-emptively expose ~everything in the Internal namespace, to make for easier experimenting for people who want to hack on Halide itself. I'm not unsympathetic to this idea, but this PR doesn't attempt to do that.
  • We take a fast-and-loose approach about implementation public API in our header files, sometimes as explicitly inline functions, sometimes not (and sometimes with constructs like inline HALIDE_NO_USER_CODE_INLINE). IMHO, we should take a hard line here and institute something like:
    • Anything in the intended public API of Halide should have its function body in a .cpp file, not a .h file, so there can be no ambiguity about where the function body is being instantiated. (Do we have things in our public API that legitimately need to be inlined for performance reasons?)
    • Templated classes and functions should be kept as 'thin' as possible in the Halide API; ideally, these should all be mostly-inlined wrappers around non-templated class/functions.
    • I'm tempted to say that HALIDE_NO_USER_CODE_INLINE is too confusing to exist and we should find a way to eradicate it. Would like to hear arguments pro or con.

I will likely be de-inlining many things (via separate PRs) as a precondition to eventually landing this.

Unfortunately, this PR doesn't solve our LLVM symbol issue at all (ie, public LLVM symbols leaking into libHalide visibility). As it turns out, specifying -fvisibility-hidden for the linker doesn't affect static libraries. We may need to compile LLVM in some special way (assuming such a way exists), or to resort to more-specific shenanigans to munge the object files after the fact, which would be painful but doable (but would likely require separate implementions for osx/linux/windows).

@abadams
Copy link
Copy Markdown
Member

abadams commented Jan 20, 2021

Not exposing everything in internal means we're going to break a lot of peoples' research code. I really think we need a test that checks that everything declared in Halide.h is exported. And if this isn't hiding stuff not in Halide.h (the llvm symbols)... what's the benefit?

steven-johnson added a commit that referenced this pull request Jan 20, 2021
As a precondition for #5655, we want to ensure that function bodies are reliably instantiated in libHalide (rather than in user code). To that end, the transformations I'm proposing:

- All functions/methods that are part of the Halide API (or "adjacent", e.g. necessary base classes or helper functions) should have their bodies in a .cpp file, not in a .h file. This includes ctors and dtors that are marked as '=default'. This also includes even "trivial" get/set functions and the like.

- All template classes that are part of the Halide API should be small enough to mark as HALIDE_ALWAYS_INLINE without fear of code explosion. (If/when we encounter such API that can't be refactored to be a thin layer around non-templated code, we may need to rethink this.)

- HALIDE_NO_USER_CODE_INLINE should just go away entirely.

This specific PR does these transformations on Func and Pipeline.
@steven-johnson
Copy link
Copy Markdown
Contributor Author

Withdrawing in favor of #5659. (Keeping branch open for my personal reference though, so please don't delete it.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants