-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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.)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.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).