Add a warning if a Generator declares any Outputs before the final Input (Fixes #7669)

python_bindings/src/halide/_generator_helpers.py

@@ -10,6 +10,7 @@
 import builtins
 import re
 import sys
+import warnings
 
 def _fail(msg: str):
     raise HalideError(msg)
@@ -487,6 +488,9 @@ def __init__(self, generator_params: dict = {}):
             for k, v in generator_params.items():
                 self._set_generatorparam_value(k, v)
 
+    def allow_out_of_order_inputs_and_outputs(self):
+        return False
+
     def configure(self):
         pass
 
@@ -567,11 +571,22 @@ def _advance_to_io_created(self):
         self._outputs_dict = {}
         self._arginfos_in = []
         self._arginfos_out = []
+        outputs_seen = False
         for name, io in _unsorted_cls_dir(self.__class__):
             is_input = isinstance(io, (InputBuffer, InputScalar))
             is_output = isinstance(io, (OutputBuffer, OutputScalar))
             if not (is_input or is_output):
                 continue
+
+            if is_input and outputs_seen and not self.allow_out_of_order_inputs_and_outputs():
+                io_order_warning = ("Generators will always produce code that orders all Inputs before all Outputs; "
+                                   "this Generator declares the Inputs and Outputs in a different order, so the calling convention may not be as expected. "
+                                   "A future version of Halide will make this illegal, and require all Inputs to be declared before all Outputs. "
+                                   "(You can avoid this requirement by overriding Generator::allow_out_of_order_inputs_and_outputs().)")
+                warnings.warn(io_order_warning)
+
+            if is_output:
+                outputs_seen = True
             d = self._inputs_dict if is_input else self._outputs_dict
             a = self._arginfos_in if is_input else self._arginfos_out
             self._add_gpio(name, io, d, a)

python_bindings/src/halide/halide_/PyGenerator.cpp

@@ -59,6 +59,10 @@ class PyGeneratorBase : public AbstractGenerator {
         return args_to_vector<ArgInfo>(generator_.attr("_get_arginfos")());
     }
 
+    bool allow_out_of_order_inputs_and_outputs() const override {
+        return generator_.attr("allow_out_of_order_inputs_and_outputs")().cast<bool>();
+    }
+
     void set_generatorparam_value(const std::string &name, const std::string &value) override {
         generator_.attr("_set_generatorparam_value")(name, value);
     }

src/AbstractGenerator.h

@@ -177,6 +177,14 @@ class AbstractGenerator {
      */
     virtual bool emit_cpp_stub(const std::string &stub_file_path) = 0;
 
+    /** By default, a Generator must declare all Inputs before all Outputs.
+     *  In some unusual situations (e.g. metaprogramming situations), it's
+     * desirable to allow them to be declared out-of-order and put the onus
+     * of a non-obvious call order on the coder; a Generator may override this
+     * to return 'true' to allow this behavior.
+     */
+    virtual bool allow_out_of_order_inputs_and_outputs() const = 0;
+
     // Below are some concrete methods that build on top of the rest of the AbstractGenerator API.
     // Note that they are nonvirtual. TODO: would these be better as freestanding methods that
     // just take AbstractGeneratorPtr as arguments?

src/Generator.cpp

@@ -1218,10 +1218,6 @@ GeneratorBase::~GeneratorBase() {
 }
 
 GeneratorParamInfo::GeneratorParamInfo(GeneratorBase *generator, const size_t size) {
-    std::vector<void *> vf = ObjectInstanceRegistry::instances_in_range(
-        generator, size, ObjectInstanceRegistry::FilterParam);
-    user_assert(vf.empty()) << "ImageParam and Param<> are no longer allowed in Generators; use Input<> instead.";
-
     const auto add_synthetic_params = [this, generator](GIOBase *gio) {
         const std::string &n = gio->name();
         const std::string &gn = generator->generator_registered_name;
@@ -1239,45 +1235,68 @@ GeneratorParamInfo::GeneratorParamInfo(GeneratorBase *generator, const size_t si
         }
     };
 
-    std::vector<void *> vi = ObjectInstanceRegistry::instances_in_range(
-        generator, size, ObjectInstanceRegistry::GeneratorInput);
-    for (auto *v : vi) {
-        auto *input = static_cast<Internal::GeneratorInputBase *>(v);
-        internal_assert(input != nullptr);
-        user_assert(is_valid_name(input->name())) << "Invalid Input name: (" << input->name() << ")\n";
-        user_assert(!names.count(input->name())) << "Duplicate Input name: " << input->name();
-        names.insert(input->name());
-        internal_assert(input->generator == nullptr || input->generator == generator);
-        input->generator = generator;
-        filter_inputs.push_back(input);
-        add_synthetic_params(input);
-    }
-
-    std::vector<void *> vo = ObjectInstanceRegistry::instances_in_range(
-        generator, size, ObjectInstanceRegistry::GeneratorOutput);
-    for (auto *v : vo) {
-        auto *output = static_cast<Internal::GeneratorOutputBase *>(v);
-        internal_assert(output != nullptr);
-        user_assert(is_valid_name(output->name())) << "Invalid Output name: (" << output->name() << ")\n";
-        user_assert(!names.count(output->name())) << "Duplicate Output name: " << output->name();
-        names.insert(output->name());
-        internal_assert(output->generator == nullptr || output->generator == generator);
-        output->generator = generator;
-        filter_outputs.push_back(output);
-        add_synthetic_params(output);
-    }
-
-    std::vector<void *> vg = ObjectInstanceRegistry::instances_in_range(
-        generator, size, ObjectInstanceRegistry::GeneratorParam);
-    for (auto *v : vg) {
-        auto *param = static_cast<GeneratorParamBase *>(v);
-        internal_assert(param != nullptr);
-        user_assert(is_valid_name(param->name())) << "Invalid GeneratorParam name: " << param->name();
-        user_assert(!names.count(param->name())) << "Duplicate GeneratorParam name: " << param->name();
-        names.insert(param->name());
-        internal_assert(param->generator == nullptr || param->generator == generator);
-        param->generator = generator;
-        filter_generator_params.push_back(param);
+    const char *const io_order_warning = "Generators will always produce code that orders all Inputs before all Outputs; "
+                                         "this Generator declares the Inputs and Outputs in a different order, so the calling convention may not be as expected. "
+                                         "A future version of Halide will make this illegal, and require all Inputs to be declared before all Outputs. "
+                                         "(You can avoid this requirement by overriding Generator::allow_out_of_order_inputs_and_outputs().)";
+
+    bool outputs_seen = false;
+    auto vf = ObjectInstanceRegistry::instances_in_range(generator, size);
+    for (const auto &vi : vf) {
+        void *const instance = vi.first;
+        const ObjectInstanceRegistry::Kind kind = vi.second;
+        switch (kind) {
+        case ObjectInstanceRegistry::GeneratorParam: {
+            auto *param = static_cast<GeneratorParamBase *>(instance);
+            internal_assert(param != nullptr);
+            user_assert(is_valid_name(param->name())) << "Invalid GeneratorParam name: " << param->name();
+            user_assert(!names.count(param->name())) << "Duplicate GeneratorParam name: " << param->name();
+            names.insert(param->name());
+            internal_assert(param->generator == nullptr || param->generator == generator);
+            param->generator = generator;
+            filter_generator_params.push_back(param);
+            break;
+        }
+        case ObjectInstanceRegistry::GeneratorInput: {
+            if (outputs_seen) {
+                if (!generator->allow_out_of_order_inputs_and_outputs()) {
+                    user_error << io_order_warning;
+                }
+            }
+            auto *input = static_cast<Internal::GeneratorInputBase *>(instance);
+            internal_assert(input != nullptr);
+            user_assert(is_valid_name(input->name())) << "Invalid Input name: (" << input->name() << ")\n";
+            user_assert(!names.count(input->name())) << "Duplicate Input name: " << input->name();
+            names.insert(input->name());
+            internal_assert(input->generator == nullptr || input->generator == generator);
+            input->generator = generator;
+            filter_inputs.push_back(input);
+            add_synthetic_params(input);
+            break;
+        }
+        case ObjectInstanceRegistry::GeneratorOutput: {
+            outputs_seen = true;
+            auto *output = static_cast<Internal::GeneratorOutputBase *>(instance);
+            internal_assert(output != nullptr);
+            user_assert(is_valid_name(output->name())) << "Invalid Output name: (" << output->name() << ")\n";
+            user_assert(!names.count(output->name())) << "Duplicate Output name: " << output->name();
+            names.insert(output->name());
+            internal_assert(output->generator == nullptr || output->generator == generator);
+            output->generator = generator;
+            filter_outputs.push_back(output);
+            add_synthetic_params(output);
+            break;
+        }
+        case ObjectInstanceRegistry::Generator:
+            // nothing
+            break;
+        case ObjectInstanceRegistry::FilterParam:
+            user_error << "ImageParam and Param<> are no longer allowed in Generators; use Input<> instead.";
+            break;
+        default:
+            user_error << "Unexpected ObjectInstanceRegistry::Kind value in GeneratorParamInfo. " << (int)kind;
+            break;
+        }
     }
 
     for (auto &g : owned_synthetic_params) {
@@ -1510,6 +1529,10 @@ std::vector<AbstractGenerator::ArgInfo> GeneratorBase::arginfos() {
     return args;
 }
 
+bool GeneratorBase::allow_out_of_order_inputs_and_outputs() const {
+    return false;
+}
+
 std::vector<Parameter> GeneratorBase::input_parameter(const std::string &name) {
     auto *input = find_input_by_name(name);
 

src/Generator.h

@@ -3673,6 +3673,7 @@ class GeneratorBase : public NamesInterface, public AbstractGenerator {
     std::string name() override;
     GeneratorContext context() const override;
     std::vector<ArgInfo> arginfos() override;
+    bool allow_out_of_order_inputs_and_outputs() const override;
 
     void set_generatorparam_value(const std::string &name, const std::string &value) override;
     void set_generatorparam_value(const std::string &name, const LoopLevel &loop_level) override;

src/ObjectInstanceRegistry.cpp

@@ -40,9 +40,9 @@ void ObjectInstanceRegistry::unregister_instance(void *this_ptr) {
 }
 
 /* static */
-std::vector<void *> ObjectInstanceRegistry::instances_in_range(void *start, size_t size,
-                                                               Kind kind) {
-    std::vector<void *> results;
+std::vector<std::pair<void *, ObjectInstanceRegistry::Kind>>
+ObjectInstanceRegistry::instances_in_range(void *start, size_t size) {
+    std::vector<std::pair<void *, ObjectInstanceRegistry::Kind>> results;
 
     ObjectInstanceRegistry &registry = get_registry();
     std::lock_guard<std::mutex> lock(registry.mutex);
@@ -52,9 +52,7 @@ std::vector<void *> ObjectInstanceRegistry::instances_in_range(void *start, size
 
     uintptr_t limit_ptr = ((uintptr_t)start) + size;
     while (it != registry.instances.end() && it->first < limit_ptr) {
-        if (it->second.kind == kind) {
-            results.push_back(it->second.subject_ptr);
-        }
+        results.emplace_back(it->second.subject_ptr, it->second.kind);
 
         if (it->first > (uintptr_t)start && it->second.size != 0) {
             // Skip over containers that we enclose

src/ObjectInstanceRegistry.h

@@ -10,7 +10,6 @@
 
 #include <cstddef>
 #include <cstdint>
-
 #include <map>
 #include <mutex>
 #include <vector>
@@ -58,11 +57,11 @@ class ObjectInstanceRegistry {
     static void unregister_instance(void *this_ptr);
 
     /** Returns the list of subject pointers for objects that have
-     *   been directly registered within the given range. If there is
-     *   another containing object inside the range, instances within
-     *   that object are skipped.
+     * been directly registered within the given range. If there is
+     * another containing object inside the range, instances within
+     * that object are skipped.
      */
-    static std::vector<void *> instances_in_range(void *start, size_t size, Kind kind);
+    static std::vector<std::pair<void *, Kind>> instances_in_range(void *start, size_t size);
 
 private:
     static ObjectInstanceRegistry &get_registry();

src/autoschedulers/adams2019/cost_model_generator.cpp

@@ -118,6 +118,11 @@ struct ModelWeight<true> : public GeneratorInput<Buffer<float>> {
 
 template<bool training>
 class CostModel : public Generator<CostModel<training>> {
+protected:
+    bool allow_out_of_order_inputs_and_outputs() const override {
+        return true;
+    }
+
 public:
     template<typename T>
     using Input = GeneratorInput<T>;

src/autoschedulers/anderson2021/cost_model_generator.cpp

@@ -118,6 +118,11 @@ struct ModelWeight<true> : public GeneratorInput<Buffer<float>> {
 
 template<bool training>
 class CostModel : public Generator<CostModel<training>> {
+protected:
+    bool allow_out_of_order_inputs_and_outputs() const override {
+        return true;
+    }
+
 public:
     template<typename T>
     using Input = GeneratorInput<T>;

test/generator/abstractgeneratortest_generator.cpp

@@ -69,6 +69,10 @@ class AbstractGeneratorTest : public AbstractGenerator {
         };
     }
 
+    bool allow_out_of_order_inputs_and_outputs() const override {
+        return false;
+    }
+
     void set_generatorparam_value(const std::string &name, const std::string &value) override {
         _halide_user_assert(!pipeline_.defined());
         _halide_user_assert(constants_.count(name) == 1) << "Unknown Constant: " << name;