AOT-generated code should include Argument information

[steven-johnson]

It would be useful if AOT-generated Halide filters included a way to introspect the expected input and output arguments.

Strawman proposal: Add simple array-of-struct data structures with names that match the filter. e.g.:

struct HalideArgumentDescriptor {
  // Halide::Type isn't really available at runtime when running AOT;
  // we'll just sorta replicate it here.
  enum Type { kInt = 0, kUint = 1, kFloat = 2, kHandle = 3 };
  const char* const name;
  const bool is_buffer;
  const Type type_code;
  const uint8_t type_bits;
  const uint8_t buffer_dimensions;
  const double scalar_default;
  const bool has_scalar_minmax;
  const double scalar_min, scalar_max;
};
struct HalideArguments {
  const int num_inputs;
  const HalideArgumentDescriptor* inputs;
  const int num_outputs;
  const HalideArgumentDescriptor* outputs;
};

// If the filter we generate is like so:
// extern "C" int my_awesome_filter(buffer_t* in1, buffer_t* in2, int16_t i, float f, buffer_t* out);

// We'd generate something like:
extern "C" HalideArguments my_awesome_filter_halide_arguments = {
  { 
    /* inputs */ 
    4, 
    {
      { "in1", true, kUInt, 8, 3, 0.0, false, 0.0, 0.0 },
      { "in2", true, kUInt, 8, 3, 0.0, false, 0.0, 0.0 },
      { "i", false, kInt, 16, 0, 0.0, false, 0.0, 0.0 },
      { "f", false, kFloat, 32, 0, 0.0, false, 0.0, 0.0 },
    }
    /* outputs */
    1,
    {
      {"out", true, kUInt, 8, 3, 0.0, false, 0.0, 0.0 },
    }
    }
  }
};

Since we'd just be adding a new extern name that no one is likely to ever be looking for, existing code should be unaffected. It's a POD of modest size so addition to code size should be unimportant.

Specific layout and contents of the descriptor-struct open for discussion, of course. The one given above is similar to one I'm using in a private branch that suits my purposes.

[abadams]

+1

I'd make the scalar_* fields a union over the possible types, so we can represent e.g. uint64_t default values.

I'd also reorder the HalideArguments struct to put both ints first, because struct packing disagreements are not a class of bug I enjoy fixing.

Our other types in the C runtime interface are of the style my_awesome_type_t. This should probably be like that too.

If the type is a serialization of Internal::ParameterContents (which it looks like it mostly is), then the min, stride, and extent buffer constraints are missing. I guess you couldn't depend on them though, because there are often non-const constraints (e.g. width = (width/8)*8). I'm on the fence about whether these should be in here. If we add them, it would be nice to add them in a way that doesn't break when we have buffers of dimensionality > 4.

On Fri, Feb 13, 2015 at 11:48 AM, Steven Johnson notifications@github.com wrote:

It would be useful if AOT-generated Halide filters included a way to introspect the expected input and output arguments.

Strawman proposal: Add simple array-of-struct data structures with names that match the filter. e.g.:

struct HalideArgumentDescriptor { // Halide::Type isn't really available at runtime when running AOT; // we'll just sorta replicate it here. enum Type { kInt = 0, kUint = 1, kFloat = 2, kHandle = 3 }; const char* const name; const bool is_buffer; const Type type_code; const uint8_t type_bits; const uint8_t buffer_dimensions; const double scalar_default; const bool has_scalar_minmax; const double scalar_min, scalar_max; }; struct HalideArguments { const int num_inputs; const HalideArgumentDescriptor* inputs; const int num_outputs; const HalideArgumentDescriptor* outputs; };

// If the filter we generate is like so: // extern "C" int my_awesome_filter(buffer_t* in1, buffer_t* in2, int16_t i, float f, buffer_t* out);

// We'd generate something like: extern "C" HalideArguments my_awesome_filter_halidearguments = { { /* inputs / 4, { { "in1", true, kUInt, 8, 3, 0.0, false, 0.0, 0.0 }, { "in2", true, kUInt, 8, 3, 0.0, false, 0.0, 0.0 }, { "i", false, kInt, 16, 0, 0.0, false, 0.0, 0.0 }, { "f", false, kFloat, 32, 0, 0.0, false, 0.0, 0.0 }, } /_ outputs */ 1, { {"out", true, kUInt, 8, 3, 0.0, false, 0.0, 0.0 }, } } } };

Since we'd just be adding a new extern name that no one is likely to ever be looking for, existing code should be unaffected. It's a POD of modest size so addition to code size should be unimportant.

Specific layout and contents of the descriptor-struct open for discussion, of course. The one given above is similar to one I'm using in a private branch that suits my purposes.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664.

[abadams]

halide_filter_arguments should probably be halide_filter_metadata, and include other things like const char *target

[abestephensg]

+1

It would be useful to include information like what Halide target string was used to build the function, etc.

I wonder at what point it would be better to return a string (e.g. a JSON or similar) to encode this info?

[steven-johnson]

I wonder at what point it would be better to return a string (e.g. a JSON or similar) to encode this info?

I'd vote "never", since that would mean the caller would have to embed a JSON-or-similar parser to make use of it, while an array-of-struct allows for simplicity.

If we're concerned about versioning, we could just add new names for new metadata (at the cost of replication).

[steven-johnson]

+1 to calling it "metadata" and including the target string.

(I don't suppose we have an existing chunk of code that uses LLVM to create and fill in a const global data structure? The LLVM documentation is daunting, and I'm eager to recycle a recipe if we already have one in our code)

[abadams]

CodeGen::create_constant_binary_blob may be a useful reference for making global constants. You probably want to use llvm::ConstantStruct::get to build the initializer, and the method in Closure::build_type to make the type.

• Andrew

On Fri, Feb 13, 2015 at 2:55 PM, Steven Johnson notifications@github.com wrote:

+1 to calling it "metadata" and including the target string.

(I don't suppose we have an existing chunk of code that uses LLVM to create and fill in a const global data structure? The LLVM documentation is daunting, and I'm eager to recycle a recipe if we already have one in our code)

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74341786.

[zvookin]

I see a lot of low-level details here, but no use case. How do we know if this actually works for an intended purpose?

Most of the cases where I've needed this sort of thing either a) need the metadata to be accessible in the compilation time environment of C++ so types can be derived from the filter function's types or b) are somewhat specific to a particular system in a way that means I effectively want an alternative to compile_to_header which generates code in a language other than C.

Use cases I can imagine for metadata include:

1) Automated test generation. 2) Supporting registration of generated functions into an interpreter of some sort. 3) Generating doc strings from a command line tool based on the generated code.

In all cases the proposed mechanism strikes me as perhaps helpful, but not complete. I think at least some minimal use must be sketched out.

A while back I put a lot of thought into what we could generate into the header to make all the information about a filter available at C++ compile time. The only answer I came up with that really worked for all use cases was to use C preprocessor macros. (E.g. it would allow building the structure proposed above by defining a macro that expanded to the initializer for the struct.) This is of course far too ugly to really deploy. However, using compile time available mechanisms, e.g. template metaprogramming of one sort or another, one ends up with a much more powerful and flexible mechanism.

I'd also consider whether we want to Move toward generating information like into another file instead of including it in the .o/.h output for Halide.

[zvookin]

Per the details already posted:

1) We need a representation of type in the new buffer_t. We might as well surface this information as its own fundamental type at the top-level now.

2) Per representing values (e.g. default/min/max), these should likely be done with a variant tagged with the type mentioned in the previous item. This is also a very useful fundamental thing. E.g. I want to have a way to call a generated filter by passing a map from string to value for all the arguments. That would use this variant type. (The more primitive thing which is really sorely needed is to call JITted code in a thread safe manner. To provide type checking the variant will be needed, even if one doesn't like using a map from string to value.)

3) I'd like the structure to at least feel like a vector/map based thing for ease of consumption. If it is solely done in the .h file, it could actually just use vector and map. (This also has the property of making it zero cost if not used. Basically an inline function builds and returns the data.) Barring that, the struct should provide appropriate methods to support things like range based for loops, etc. (These can be ifdef'ed to C++ or the whole mechanism can be made C++ only.)

[steven-johnson]

Re: the details:

1) I'd be in agreement with that; we can go ahead and define it in buffer_t.h even if it's not (yet) being used by buffer_t.

2) tagged variant union is definitely a better option for this; the struct proposed above was (admittedly) a quick strawman based on some local work I've done.

3) this brings up a more interesting point, which is whether it should be provided as constant data baked into the .o file (which was my proposal), or as a separate .h and/or .cc file in source form. My knee-jerk reaction was that since the filter API is extern "C", the metadata should also conform to plain "C"... but upon further reflection, it seems mighty unlikely that anyone consuming it won't have access to C++.

If automated-test and interpreter-function-registration are desirable goals (and I think they are), then perhaps what we want is to combine this with a register-by-name approach similar to RegisterGenerator, but implemented at runtime. Strawman:

• ${FILTER}.h/${FILTER}.o continue to be generated as-is
• optional generation of new files, named "${FILTER}_metadata.h" and "${FILTER}_metadata.cpp"
• ${FILTER}metadata.h contains something like: inline map<string, variant> get${FILTER}_metadata() { ... build and return map }
• ${FILTER}metadata.cpp contains something like: RegisterFilter register${FILTER}("filter", ${FILTER}, get_${FILTER}_metadata());
• Add new functions to halide runtime like: struct FilterInfo { const char* name; int (_filterfunc)(void* args); // _jit_wrapper argument form map<string, variant> metadata } bool halide_find_filter(const char* name, FilterInfo* info); bool halide_enum_filters(void (callback)(const FilterInfo info));

So:

• code that simply wants the metadata for a filter and knows about it at compile/build time can include the _metadata.h and need not use the registration mechanism.
• code that needs to introspect arbitrary filters that can be glommed together at link time just has to require that the _metadata.cpp files are included in the build.

[abadams]

The use-case I was thinking of was plugin registration (e.g. matlab, scipy). The original proposal covers this case nicely. You'd be able to take a precompiled shared library full of halide-generated routines and call them, without the library author being aware of the application you're plugging these into. You just need to know the name of the function you want, so that you can dlsym the related symbols.

We're planning to have such a library, and it would be nice if people could make use of it from languages and environments that we don't plan to target directly without having to wrap each individual function.

Exposing the argument metadata to C++ at compile time via the type system seems like it might be a cool thing, as does providing accessors to parse the metadata and return it in more C++-friendly data structures, but I have no use-cases for either, and they can be easily added later if there is a pressing one.

[steven-johnson]

The meta-point here is that I agree with Zalman: we should actually consider the use cases before proposing a solution :-) Then of course come up with the simplest thing that seems likely to address them. Embedding a simple data structure (and providing optional C++ wrappers to unwind 'em into map, etc when necessary) might fit that bill.

Any other thoughts on this? @dsharletg -- I suspect you may have some thoughts on the matter,

[dsharletg]

I've been thinking about this since you posted it. First of all, what I was planning to do with the matlab plugin generator was mostly independent of this, the real piece of information I was missing for that was more type info (dimensionality, etc.) in the Argument class (i.e. at Halide compile time). However, after talking with Andrew, I could see it working using this data instead, and it would be simpler in a lot of ways.

As for the actual data itself, I don't have too strong of an opinion about how its designed (struct vs. C++ data structure, etc.). If I were designing it from scratch, I'd be tempted to consider a design where there were just a whole bunch of exported symbols at the lowest level. Suppose the pipeline is named 'fn', then I would export symbols like:

int fn_input_count
int fn_output_count
const char *fn_input0_name
bool fn_input0_is_buffer
...
const char *fn_output0_name
...

Then, if we wanted to, we could provide helper functions to load all that information into a struct/C++ map/etc given a particular pipeline name. It seems like this might be marginally more future proof than exporting a struct, and it doesn't rely on C++ at the lowest common denominator level.

[steven-johnson]

I think exporting a single symbol (pointing at a struct-like thing) will be easier to deal with and a lot less polluting of the exported names. If we need to extend it in the future, we can always add additional names.

[steven-johnson]

(I will say that the idea of producing the metadata as C/C++ code, rather than via LLVM generation, is a bit seductive, since it would be easier to inspect the generated result... but I'm probably just telling myself that because it would be easier to generate :-)

[steven-johnson]

A few folks discussed this more at length locally; here were what I think the takeaways were:

• It's probably desirable that code could dlsym some symbol to get the metadata for a loaded file; this argues against using std::map<string, variant> as the intrinsic storage format for the metadata (since there's no guarantee that std::map will have compatible implementations across the boundary). Expressing it as a C struct (with careful attention to packing, alignment, etc) is probably the most portable approach. (Providing utilities to convert between this and a more C++-friendly std::map structure would be desirable.)
• It's desirable to not have the metadata included in a generated .o by default, since many applications won't want it, and would prefer to avoid having to rely on dead-stripping to remove it. It's also desirable to not have generation controlled by another build or Target flag (since that turned out to be a big headache for user-context). Instead, let's generate the metadata as a separate C source file (either a .h or .c, details TBD) produced by Generator::emit_filter(); build systems that want to use the metadata just need to include this new file in the build manifest, and those that don't, don't.
• Don't attempt (yet) to provide overall policy for a registry-based filter setup (as I mentioned as a thought in an earlier comment); we may find that desirable at a later date, but that mechanism should be buildable (and built) on top of this one.

[abadams]

I disagree with point two. The thinking behind our runtime is that a small amount of extra data in the .o is better than a more complex build setup. Most of the runtime is unused by most apps (tiff writing, memoization, tracing...), so we're already expecting people to either not care about object size or to strip it.

Also, the a library author may not have cared about producing this metadata, but the person trying to use their compiled code dynamically will want it. So on by default seems better than off by default. If someone really objects to having it in their .o, they can strip the symbol.

user_context was messy because we didn't want to make it mandatory. I think this should be mandatory.

On Tue, Feb 17, 2015 at 12:31 PM, Steven Johnson notifications@github.com wrote:

A few folks discussed this more at length locally; here were what I think the takeaways were:

-

It's probably desirable that code could dlsym some symbol to get the metadata for a loaded file; this argues against using std::map as the intrinsic storage format for the metadata (since there's no guarantee that std::map will have compatible implementations across the boundary). Expressing it as a C struct (with careful attention to packing, alignment, etc) is probably the most portable approach. (Providing utilities to convert between this and a more C++-friendly std::map structure would be desirable.)

It's desirable to not have the metadata included in a generated .o by default, since many applications won't want it, and would prefer to avoid having to rely on dead-stripping to remove it. It's also desirable to not have generation controlled by another build or Target flag (since that turned out to be a big headache for user-context). Instead, let's generate the metadata as a separate C source file (either a .h or .c, details TBD) produced by Generator::emit_filter(); build systems that want to use the metadata just need to include this new file in the build manifest, and those that don't, don't.

Don't attempt (yet) to provide overall policy for a registry-based filter setup (as I mentioned as a thought in an earlier comment); we may find that desirable at a later date, but that mechanism should be buildable (and built) on top of this one.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74746956.

[zvookin]

Andrew, let me clarify the point about dead stripping:

If one wants the metadata to be there for dlsym on a plug-in shared object, the symbol for the metadata structure cannot be dead stripped. The following are ways to avoid dead stripping:

1) The name of the metadata structure must be mentioned in some sort of exports flag or file to the linker. 2) The metadata structure must be accessed by the filter code itself. 3) The metadata structure must be marked with an export attribute to disable dead stripping.

The first choice requires some extra build logic mentioning the filters name, the second two make if very difficult to get rid of the information. And having the metadata is not just a code size thing as it carries extra information about the filter that people developing non-open-source code may not wish to place in their binary.

A proposed solution is to put the metadata in a separate .o (or C file perhaps) and mark it in such a way that it will not get dead stripped. The end user controls whether to include the info or not by adding an extra file to their build.

On Tue, Feb 17, 2015 at 12:49 PM, Andrew Adams notifications@github.com wrote:

I disagree with point two. The thinking behind our runtime is that a small amount of extra data in the .o is better than a more complex build setup. Most of the runtime is unused by most apps (tiff writing, memoization, tracing...), so we're already expecting people to either not care about object size or to strip it.

Also, the a library author may not have cared about producing this metadata, but the person trying to use their compiled code dynamically will want it. So on by default seems better than off by default. If someone really objects to having it in their .o, they can strip the symbol.

user_context was messy because we didn't want to make it mandatory. I think this should be mandatory.

On Tue, Feb 17, 2015 at 12:31 PM, Steven Johnson <notifications@github.com

wrote:

A few folks discussed this more at length locally; here were what I think the takeaways were:

It's probably desirable that code could dlsym some symbol to get the metadata for a loaded file; this argues against using std::map as the intrinsic storage format for the metadata (since there's no guarantee that std::map will have compatible implementations across the boundary). Expressing it as a C struct (with careful attention to packing, alignment, etc) is probably the most portable approach. (Providing utilities to convert between this and a more C++-friendly std::map structure would be

desirable.)

It's desirable to not have the metadata included in a generated .o by default, since many applications won't want it, and would prefer to avoid having to rely on dead-stripping to remove it. It's also desirable to not have generation controlled by another build or Target flag (since that turned out to be a big headache for user-context). Instead, let's generate the metadata as a separate C source file (either a .h or .c, details TBD) produced by Generator::emit_filter(); build systems that want to use the metadata just need to include this new file in the build

manifest, and those that don't, don't.

Don't attempt (yet) to provide overall policy for a registry-based filter setup (as I mentioned as a thought in an earlier comment); we may find that desirable at a later date, but that mechanism should be buildable (and built) on top of this one.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74746956.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74750228.

[abadams]

Ah, I was thinking of static compilation and stripping the final binary. For libraries, marking it as export (3) seems fine to me.

If you want to limit peoples ability to introspect on your compiled code, it's easy enough to strip out with a single-line "strip my_object.o -w -N '.*_metadata'" build rule.

I believe including the metadata should be the common case, and so is the better default, so it's the one we should make simple. I'm trying to resist incremental increases in complexity here.

R.e. closed-source: Is this sort of metadata sensitive enough that this should be an explicit opt-in?

On Tue, Feb 17, 2015 at 1:18 PM, Zalman Stern notifications@github.com wrote:

Andrew, let me clarify the point about dead stripping:

If one wants the metadata to be there for dlsym on a plug-in shared object, the symbol for the metadata structure cannot be dead stripped. The following are ways to avoid dead stripping:

1) The name of the metadata structure must be mentioned in some sort of exports flag or file to the linker. 2) The metadata structure must be accessed by the filter code itself. 3) The metadata structure must be marked with an export attribute to disable dead stripping.

The first choice requires some extra build logic mentioning the filters name, the second two make if very difficult to get rid of the information. And having the metadata is not just a code size thing as it carries extra information about the filter that people developing non-open-source code may not wish to place in their binary.

A proposed solution is to put the metadata in a separate .o (or C file perhaps) and mark it in such a way that it will not get dead stripped. The end user controls whether to include the info or not by adding an extra file to their build.

On Tue, Feb 17, 2015 at 12:49 PM, Andrew Adams notifications@github.com wrote:

I disagree with point two. The thinking behind our runtime is that a small amount of extra data in the .o is better than a more complex build setup. Most of the runtime is unused by most apps (tiff writing, memoization, tracing...), so we're already expecting people to either not care about object size or to strip it.

Also, the a library author may not have cared about producing this metadata, but the person trying to use their compiled code dynamically will want it. So on by default seems better than off by default. If someone really objects to having it in their .o, they can strip the symbol.

user_context was messy because we didn't want to make it mandatory. I think this should be mandatory.

On Tue, Feb 17, 2015 at 12:31 PM, Steven Johnson < notifications@github.com

wrote:

A few folks discussed this more at length locally; here were what I think the takeaways were:

It's probably desirable that code could dlsym some symbol to get the metadata for a loaded file; this argues against using std::map as the intrinsic storage format for the metadata (since there's no guarantee that std::map will have compatible implementations across the boundary). Expressing it as a C struct (with careful attention to packing, alignment, etc) is probably the most portable approach. (Providing utilities to convert between this and a more C++-friendly std::map structure would be

desirable.)

It's desirable to not have the metadata included in a generated .o by default, since many applications won't want it, and would prefer to avoid having to rely on dead-stripping to remove it. It's also desirable to not have generation controlled by another build or Target flag (since that turned out to be a big headache for user-context). Instead, let's generate the metadata as a separate C source file (either a .h or .c, details TBD) produced by Generator::emit_filter(); build systems that want to use the metadata just need to include this new file in the build

manifest, and those that don't, don't.

Don't attempt (yet) to provide overall policy for a registry-based filter setup (as I mentioned as a thought in an earlier comment); we may find that desirable at a later date, but that mechanism should be buildable (and built) on top of this one.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74746956.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74750228.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74755903.

[dsharletg]

How about just making the metadata default to on, and having a target flag to turn it off? i.e. a flag NoMetadata, alongside NoAsserts, NoBoundsQuery, ...

edit: I see now the comment about target flags above. However, I think this is different from user context. User context is tricky because you get compatibility issues between pipelines that use it vs. those that don't. In the case of metadata, I can't see any issue with mixing pipelines that have metadata and those that don't.

[abadams]

That sounds fine to me. Default on, but move the choice outside of the build setup entirely. Though Zalman points out offline that you're going to have to at least list the symbol in an exports file somewhere if your default symbol visibility is not public.

On Tue, Feb 17, 2015 at 2:02 PM, Dillon Sharlet notifications@github.com wrote:

How about just making the metadata default to on, and having a target flag to turn it off? i.e. a flag NoMetadata, alongside NoAsserts, NoBoundsQuery, ...

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74764005.

[steven-johnson]

The metadata structure must be marked with an export attribute to disable dead stripping

I'm a little unclear about how this will work in practice. In particular, if I link this .o into an executable, but never reference the _metadata, I'd expect --gc-sections to strip the symbol. Yes?

[steven-johnson]

You probably want to use ... the method in Closure::build_type to make the type.

Actually, wouldn't it be preferable to use the same approach as buffer_t (define it in the initial module via source inclusion, then extract a reference to the definition)?

[abadams]

Oh yeah, that's a better approach. You only need the llvm api way if the struct contents vary, as they do with the closure.

On Wed, Feb 18, 2015 at 10:48 AM, Steven Johnson notifications@github.com wrote:

You probably want to use ... the method in Closure::build_type to make the type.

Actually, wouldn't it be preferable to use the same approach as buffer_t (define it in the initial module via source inclusion, then extract a reference to the definition)?

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-74922619.

[steven-johnson]

One more interesting fly in the ointment: right now, all of the Func::compile_xxx() methods use Argument (or vector) to represent the arguments; unfortunately, Argument is inadequate for our needs (it's missing buffer dimensions, scalar defaults/min/max).

Option 1: add these to Argument. Downside: now somewhat redundant to Internal::Parameter; must track down everything that constructs Argument to ensure that the new fields are plausibly filled in.

Option 2: use Internal::Parameter for the path(s) that need to generate metadata. Downside: it's a lot of paths; these are public APIs and using Internal::Parameter is distasteful.

I think Option 1 is the better choice, but it's a nontrivial-enough change that I need to see what people think before moving forward. (Also, if I go that direction, I'll probably do it as a separate pull request to minimize the churn.)

[dsharletg]

In the past, I've definitely wanted more information in Argument (dimensionality and type of ImageParam at the very least). I lean toward option 1. Last time I looked at this, I even thought about just moving everything to Argument and getting rid of Internal::Parameter, but that looked difficult last time I looked at it.

On your last point, I actually think it would make sense to put these related changes into one PR. It would be easier to see all these related changes at once. That said, I don't have any objection to merging the one that's already ready to merge (#676).

[abadams]

With option 1, it looks like the only difference between an Argument and an Internal::Parameter is that a Parameter can be bound to a value or buffer for jitting.

From the user's point of view, Param and ImageParam are both Arguments and can be implicitly cast to those. From the point of view of the internals, Param and ImageParam are both user-friendly views of a Parameter.

Argument is a user-facing struct, so it's hard to change. Perhaps Parameter::Contents should just be an Argument plus a handful of other fields (buffer, data, ref_count, is_explicit_name, is_registered). Then converting an ImageParam or Param to an Argument would just go get the Argument field from the Parameter::Contents.

On Fri, Feb 20, 2015 at 4:01 PM, Steven Johnson notifications@github.com wrote:

One more interesting fly in the ointment: right now, all of the Func::compile_xxx() methods use Argument (or vector) to represent the arguments; unfortunately, Argument is inadequate for our needs (it's missing buffer dimensions, scalar defaults/min/max).

Option 1: add these to Argument. Downside: now somewhat redundant to Internal::Parameter; must track down everything that constructs Argument to ensure that the new fields are plausibly filled in.

Option 2: use Internal::Parameter for the path(s) that need to generate metadata. Downside: it's a lot of paths; these are public APIs and using Internal::Parameter is distasteful.

I think Option 1 is the better choice, but it's a nontrivial-enough change that I need to see what people think before moving forward. (Also, if I go that direction, I'll probably do it as a separate pull request to minimize the churn.)

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-75341863.

[steven-johnson]

Argument is a user-facing struct, so it's hard to change.

We don't have to actually change/remove any existing fields; strictly adding new fields should be all we need. Let me try that out and see how it looks.

[abadams]

That was an intermediate stage of my thought process that I didn't write down: I was weighing changing Argument to just contain a Parameter and not expose the extra stuff vs changing Parameter::Contents to just be Argument plus extra fields.

The first option was both weird from an inheritance hierarchy perspective, and also not possible without breaking user code.

On Fri, Feb 20, 2015 at 4:23 PM, Steven Johnson notifications@github.com wrote:

Argument is a user-facing struct, so it's hard to change.

We don't have to actually change/remove any existing fields; strictly adding new fields should be all we need. Let me try that out and see how it looks.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-75343854.

[steven-johnson]

Actually, wouldn't it be preferable to use the same approach as buffer_t (define it in the initial module via source inclusion, then extract a reference to the definition)?

Fun fact: unless I'm missing something, there isn't an existing runtime module that is guaranteed to be present in all configurations; looks like I'll need to add a new one so that the type gets exported in all configurations.

[abadams]

If you're just defining a type, runtime_internal.h should always be there because every module includes it.

On Mon, Mar 2, 2015 at 3:30 PM, Steven Johnson notifications@github.com wrote:

Actually, wouldn't it be preferable to use the same approach as buffer_t (define it in the initial module via source inclusion, then extract a reference to the definition)?

Fun fact: unless I'm missing something, there isn't an existing runtime module that is guaranteed to be present in all configurations; looks like I'll need to add a new one so that the type gets exported in all configurations.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-76850650.

[zvookin]

What are you doing that needs llvm types again?

I decided I needed to force types like buffer_t to be always defined when doing the JIT module stuff. I started out by making a module that just defined them. It turned out that there were issues between opaque types and concrete ones and issues of type uniqueness. Ultimately I bailed on trying to get all the types to exist up front. I had to make some changes to copy_llvm_type_to_module to handle the opaque types, but it was much simpler.

-Z-

On Mon, Mar 2, 2015 at 3:30 PM, Steven Johnson notifications@github.com wrote:

Actually, wouldn't it be preferable to use the same approach as buffer_t (define it in the initial module via source inclusion, then extract a reference to the definition)?

Fun fact: unless I'm missing something, there isn't an existing runtime module that is guaranteed to be present in all configurations; looks like I'll need to add a new one so that the type gets exported in all configurations.

— Reply to this email directly or view it on GitHub https://github.com/halide/Halide/issues/664#issuecomment-76850650.

[steven-johnson]

What are you doing that needs llvm types again?

Emitting an instance of a metadata struct. (It's basically the same sort of drill that we do to emit constant images in CodeGen.cpp, but with a different type.)

runtime_internal.h should always be there

Will that work if no .cpp file in the runtime modules references it? (My initial workaround was to declare a weak constant null pointer to the time I wanted... no, I'm not proud of admitting that)

[steven-johnson]

I'd make the scalar_* fields a union over the possible types

Fun fact: AFAICT, LLVM doesn't really support union types directly; online documentation / examples suggest you need to roll it yourself via bitcasting, etc. (This isn't hard to do, just more boilerplate code.) Maybe I'm missing something? Seems a rather odd omission.

[steven-johnson]

I think PR #722 has settled this.