Moved the bulk of the implementation of configure_pipeline_features() to

[benoitsteiner]

the DefaultCostModel class. This allows custom cost models more
flexibility in modeling pipeline features.

[steven-johnson]

Lots of build failures, apparently

[benoitsteiner]

It looks like the command I used to validate the PR (make test) didn't trigger a full build. I'm at a conference this week, so I'm not sure I'll be able to make much progress towards fixing the errors, but I'll have time to get back to this PR next week.

[steven-johnson]

This should not need to link in libHalide -- in general, you should only need libHalide if you are using the JIT (or you are a Generator). Why did you need to add this?

[benoitsteiner]

Without the libHalide dependency, symbols corresponding to multiple methods of the Halide::Internal::Introspection and Halide::Internal::ErrorReport::ErrorReport classes are missing. I picked up the dependency through the #include "Error.h" in FunctionDAG.h. I'm guessing that the introspection dependency came through the added dependency on the Error class.

[steven-johnson]

Ouch. We need to not do that. It is a bad code smell to need to link in libHalide just for error reporting.

[steven-johnson]

Just to be clear: we don't want to land this as-is, as libHalide should not be necessary to use in this way. We should instead fix retrain_cost_model to avoid the need forit.

[benoitsteiner]

It looks like this requires a fair amount of work to create a standalone library with basic utilities such as logging and error handling that can then be linked into various apps without having to pull in libHalide. The only other solution I can think of is to forego the Halide error handling code in the autoscheduler code and rewrite the code to either assert or throw exceptions whenever something bad happens.

[steven-johnson]

Linking in all of libHalide just to get the logging & error-handling code is definitely a code smell. I would be very reluctant to approve it.

[abadams]

Most of this code already uses aslog for logging and regular old assert for error checking. Let's just keep doing that. I think there's a new internal_assert in this PR that should just be assert

[benoitsteiner]

Removing all the #include "Errors.h" still result in missing symbols erros related to Halide::Internal::Introspection::get_source_locationabi:cxx11. It looks like this gets included from HalideBuffer.h.
Most of the autoscheduler code depends one way or another on Halide internals, so even if I manage to remove the dependency of retrain_cost_model on libHalide.so the problem will reappear sooner or later.

[abadams]

HalideBuffer.h definitely doesn't depend on libHalide - otherwise we'd be unable to ship Halide code without shipping the whole compiler.

Elsewhere in the project we have a hard line between things that depend on libHalide, and things that depend on Halide-AOT-compiled code. retrain_cost_model is supposed to be in that latter category. The autoscheduler itself is awkwardly in the middle.

[steven-johnson]

retrain_cost_model is used by the autoscheduling script, but it is a standalone tool, not part of the autoscheduler library itself. It should not depend on libHalide. If it needs to, we're doing something very wrong.

[benoitsteiner]

I just managed to trace the root cause of the problem: FunctionDAG.h includes Halide.h, which contains the following code:

#ifndef COMPILING_HALIDE
namespace Halide {
namespace Internal {
static bool check_introspection(const void *var, const std::string &type,
                                const std::string &correct_name,
                                const std::string &correct_file, int line) {
    std::string correct_loc = correct_file + ":" + std::to_string(line);
    std::string loc = Introspection::get_source_location();
    std::string name = Introspection::get_variable_name(var, type);
    return name == correct_name && loc == correct_loc;
}
}  // namespace Internal
}  // namespace Halide
#endif 

Both Introspection::get_source_location() and Introspection::get_variable_name() are declared in Halide.h but not defined, which is why we end up with these 2 missing symbols unless we link Halide.
I'm not sure that the proper way to fix this is. Adding #define COMPILING_HALIDE in FunctionDAG.h before including Halide.h would solve this problem, but it looks like an ugly hack.

[abadams]

FunctionDAG.h definitely depends on libHalide. It uses Exprs. retrain_cost_model shouldn't need FunctionDAG.h

[benoitsteiner]

I updated CostModel::set_pipeline_features to take a FunctionDAG in order to enable us extract different pipeline features from a function dag without having to fork and update the code in several places. As a result the default cost model and ultimately retrain_cost_model also depend on FunctionDAG,

[steven-johnson]

Unfortunately it sounds like some refactoring is really necessary.

[abadams]

OK, I think I get it now. This PR is a refactoring that gives cost models more flexibility in how they extract pipeline features, but in doing so it changes the cost model interface - instead of grabbing a predefined common set of pipeline features in a specific order as a Runtime::Buffer, it consumes a whole FunctionDAG. This adds all of libHalide as a dependency to anything that wants to deal with a cost model, because a FunctionDAG uses things like Exprs.

Hrm. Having cost model subclasses accept a FunctionDAG and do their own featurization is a reasonable bit of flexibility to add. I guess we either eat the new dependency, or we refactor FunctionDAG to separate the libHalide-using parts from the other parts, which seems unnecessarily difficult compared to just add a -lHalide. So Steven: I think there's a good reason for the new dependency.

[steven-johnson]

Mixing AOT and JIT code in the same runtime is treading on interesting ground. Before we go down this path, can we get a little more background on why this is a must-have feature?

[benoitsteiner]

The CostModel class allows us to experiment with different ways to predict costs given a set of pipeline features. However, with the current code we have to modify AutoScheduler.cc every time we want to experiment with different pipeline features. This makes the whole process much more difficult than it needs to be, and has forced us to fork the autoscheduler code. With the PR, we gain the flexibility we need to experiment with different pipeline features.

[steven-johnson]

Thanks for the details. I'll re-review today with that in mind.

[steven-johnson]

various formatting nits that are best solved by applying clang-format

[steven-johnson]

We almost certainly shouldn't be mixing assert() and internal_assert() in the same file; IMHO, if internal_assert is available, we should prefer it, as it has consistent meaning (doesn't rely on build settings) and has much richer failure messages.

[steven-johnson]

See above comment; if we are committed to making libHalide available here, we should use internal_error or internal_assert rather than std::cerr and/or abort.

[steven-johnson]

(Apologies for the very-belated-review!)

This PR is a refactoring that gives cost models more flexibility in how they extract pipeline features, but in doing so it changes the cost model interface - instead of grabbing a predefined common set of pipeline features in a specific order as a Runtime::Buffer, it consumes a whole FunctionDAG. This adds all of libHalide as a dependency to anything that wants to deal with a cost model, because a FunctionDAG uses things like Exprs.

Yeah, I see that now. That said: IIUC, DefaultCostModel::set_pipeline_features() doesn't actually use any of the libHalide-needing-parts of FunctionDAG; AFAICT, it walks over the nodes list to count the input stages, then extracts parts of the PipelineFeatures.

A refactoring to avoid bringing in libHalide would be much preferable and not too difficult (e.g., make an abstract class to allow accessing the PipelineFeatures, make that the input to set_pipeline_features() instead of FunctionDAG, and have FunctionDAG provide an implementation of that class (via wrapper, inheritance, etc).

Am I missing something here? Based on what I'm seeing, this still seems like an unnecessary (and undesirable) dependence.

[steven-johnson]

I've taken the liberty of prepping an alternate take on this code (#4536) that I think achieves the same goal without adding the libHalide dependency. Please take a look.

[abadams]

Discussed with Steven offline why a CostModel subclass might need all of libHalide in a way that it can't encode in the PipelineFeatures. The current state of this PR LGTM assuming it passes test. I guess we need it to clone it into the main repo to trigger full testing.