Move function/method bodies from .h -> .cpp for Func, Pipeline#5657
Move function/method bodies from .h -> .cpp for Func, Pipeline#5657steven-johnson wants to merge 5 commits intomasterfrom
Conversation
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.
| Stage &reorder(const std::vector<VarOrRVar> &vars); | ||
|
|
||
| template<typename... Args> | ||
| HALIDE_NO_USER_CODE_INLINE typename std::enable_if<Internal::all_are_convertible<VarOrRVar, Args...>::value, Stage &>::type |
There was a problem hiding this comment.
This can't safely be inlined into user code - it causes useless line numbers in error messages. We want everything in the Halide namespace that could throw a user_error to stay in the Halide namespace, and not get pasted into user code.
There was a problem hiding this comment.
useless line numbers are bad, but it's not clear to me how that causes that -- all this is doing in user code is gathering up the args into a vector, then calling the non-templated form of reorder(). Where do the useless line numbers get injected if this is inlined?
There was a problem hiding this comment.
The error message reporter walks up the stack and reports the line number associated with the first line it finds outside of the Halide:: namespace to help users pinpoint what call into libHalide threw an error. If this method body gets pasted into user code and then the underlying reorder call throws an error then you get an error message blaming Halide.h:34897 instead of your_code.cpp:87
This issue was the origin of HALIDE_NO_USER_CODE_INLINE. For a bunch of functions we wanted inline linkage (because templates), but actually inlining the bodies muddies the boundary between user code and libHalide.
There was a problem hiding this comment.
Hm, so in that case, why not use HALIDE_NEVER_INLINE?
There was a problem hiding this comment.
Digging through history:
There was a problem hiding this comment.
That explains the naming, but it doesn't explain why we are making the sometimes-inline vs never-inline choice.
But at a more meta level, I'm pondering whether this tradeoff is the right decision. The main advantage seems to be that when an error occurs, the user sees a much more helpful line number, so, that's good. But as a downside, it (IMHO) makes it hard to quickly reason about where this code will actually be instantiated -- what we're actually doing here is saying "attempt to inline this if used internally by halide, but never inline in user code". That's not unreasonable, but it leaves open the possibility of the internal-to-halide usage not actually being inlined (since it's not a 'force' inline), making it an exported symbol from libHalide... and now we have two versions of it seen by user code (the one from libHalide, plus the one instantiated in user code due to the no-inline directive). The linker will quietly choose one of these, but they could be subtly different due to different compilers being used. I dunno, maybe I'm overthinking this, but quiet unpredictabilities of this sort worry me.
There was a problem hiding this comment.
I have no problem with just making it never inline.
| * disambiguate calls to min on FuncRefs when a user has pulled both | ||
| * Halide::min and std::min into their namespace. */ | ||
| // @{ | ||
| inline Expr min(const FuncRef &a, const FuncRef &b) { |
There was a problem hiding this comment.
Does this need to stay in the header?
There was a problem hiding this comment.
Good question. Let me find out.
|
|
||
| /** Construct a new Func to wrap a Buffer. */ | ||
| template<typename T> | ||
| HALIDE_NO_USER_CODE_INLINE explicit Func(Buffer<T> &im) |
There was a problem hiding this comment.
Same issue as above. We can't inline this.
| } | ||
|
|
||
| template<typename T> | ||
| HALIDE_NO_USER_CODE_INLINE RealizationArg(Buffer<T> &dst) |
There was a problem hiding this comment.
Not sure if this one can throw an error or not. We can only use ALWAYS_INLINE if it won't.
|
Note that #5655 is now moot, but the goal of this PR is still worthwhile; it's on hold briefly while I attend to other things but also to think more about how to handle inlined template methods. |
3 participants
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.