Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 16 commits into from

Conversation

steven-johnson
Copy link
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
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
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