Revise the calling convention of the generated code for _metadata #6758
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
This PR does some groundwork necessary to implement #6757 cleanly: - #6757 uses `call_cached_indirect_function` to support multitarget cleanliness for metadata getters - but, name-mangling in MSVC includes the return type, which is currently `halide_filter_metadata_t*` for the getter - the existing code in Codegen_LLVM assumes that all the functions passed to `compile()` return int32; while this *could* probably be generalized, there is potentially a long tail of special-cases to deal with, as I discovered from some experimentation - therefore, this changes the *generated* code of the metadata getetr to match the form of the signature that this expects, ie `int foo(args)`. Before this, we'd generate an extern func like: const struct halide_filter_metadata_t * metadata_tester_get_metadata(); now, we generate an extern func like: int metadata_tester_get_metadata(const struct halide_filter_metadata_t **md_out); ...along with an inline wrapper using the old name and calling convention so that call sites don't need to change.
|
I'm converting this to a draft PR, because it turns out that inside the can of worms mentioned above lies another, even larger can of worms -- defying space-time somehow -- to wit: our codegen not only expects an int32 return type, it also expects all input/output args to be either scalar or halide_buffer_t, and trying to coerce it to flexibly supporting other things looks to be a nontrivial effort. I began this mess (along with #6757) on the assumption it would be a quick-n-easy way to assist in some debugging issues, but it manifestly is not, and since there's no compelling need for these changes that I'm aware of, I'm just gonna mothball these here for future reference. |
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 PR does some groundwork necessary to implement #6757 cleanly:
_metadata()getters should be multitarget-aware #6757 usescall_cached_indirect_functionto support multitarget cleanliness for metadata gettershalide_filter_metadata_t*for the gettercompile()return int32; while this could probably be generalized, there is potentially a long tail of special-cases to deal with, as I discovered from some experimentationint foo(args).Before this, we'd generate an extern func like:
now, we generate an extern func like:
...along with an inline wrapper using the old name and calling convention so that call sites don't need to change.