In RunGen::bounds_query_input_shapes(), use extent=1 instead of extent=0

[steven-johnson]

Surprisingly, extent=0 actually worked in a surprising number of cases, but has pathological behavior for some constraints.

[abadams]

In the absence of constraints, 0 should always work. Constraints that only depend on the output should also be fine. What sort of constraints did it break for? Why is one better than zero in those cases?

[steven-johnson]

Interesting, I (foolishly) assumed that since extent=0 isn't really a legal extent in the first place that we were getting lucky. It's failing with Applying the constraints on Input buffer image to the required region made it smaller in dimension 0. Required size: 0 to -1. Constrained size: 0 to -2.

What I suspect are the relevant parts look something like

image = (the input)
image_bounded_y(x, y) = image(x, clamp(y, image.dim(1).min(), image.dim(1).max()));  // NOT using repeat_edge
image_downsampled_y(x, y) = simple_function_of(image_bounded_y);
image_downsampled_y_bounded_x = repeat_edge(image_downsampled_y, image.dim(0).min(), image.dim(0).extent());
result = simple_summation_of(image_downsampled_y_bounded_x);

but I'll see if I can get it into a small case I can share publicly.

[steven-johnson]

Aha: it's tied up with putting constraints to align rows. Do this:

• Replace contents of example_generator.cpp with this:

#include "Halide.h"

namespace {

using namespace Halide;
using namespace Halide::BoundaryConditions;

Var x("x"), y("y");

void RequireAlignedRows(Halide::OutputImageParam param, int alignment) {
  param.dim(0).set_min((param.dim(0).min() / alignment) * alignment);
  param.dim(0).set_extent((param.dim(0).extent() / alignment) * alignment);

  for (int i = 1; i < param.dimensions(); i++) {
    param.dim(i).set_stride((param.dim(i).stride() / alignment) * alignment);
  }
}

class Example : public Halide::Generator<Example> {
public:
  Input<Buffer<uint16_t>> image_{"image", 2};

  Func build() {
    Func a;
    a(x, y) = image_(x, clamp(y, image_.dim(1).min(), image_.dim(1).max()));

    Func b = BoundaryConditions::repeat_edge(a, image_.dim(0).min(), image_.dim(0).extent());

    Func result;
    result(x, y) = b(x, y);

    // ----- Estimates
    {
      const int W = 256;
      const int H = 256;
      image_.set_estimates({{0, W}, {0, H}});
      result.set_estimates({{0, W}, {0, H}});
    }

    // Schedule.
    if (!auto_schedule) {
      RequireAlignedRows(image_, 8);
      result.compute_root();
    }

    return result;
  }
};

}  // namespace

HALIDE_REGISTER_GENERATOR(Example, example)

Then do this:

make bin/host/build/example.run RUNARGS="--benchmarks=all --estimate_all"

Result: halide_error: Applying the constraints on Input buffer image to the required region made it smaller in dimension 0. Required size: 0 to -1. Constrained size: 0 to -2.

My change to start with extent=1 just happens to make this case happy, but that's clearly just dumb luck.

[abadams]

It's a good idea to write constraints that round things up, rather than down, so that they don't fight bounds queries this way. Throwing +1s in the numerator would mean the same thing and work. That's not a very satisfying fix either though. You can set up constraint systems that are arbitrarily difficult to solve in a single bounds query.

[steven-johnson]

(Closing since the 'fix' is clearly bogus)

[steven-johnson]

That's not a very satisfying fix either though. You can set up constraint systems that are arbitrarily difficult to solve in a single bounds query.

Thought: what if the code in RunGen checked (specifically) for the bounds-too-small error, and when seeing that, iterated over other values (odd-vs-even) to try to work around this? Yeah, it's a hack, but it might be a reasonable workaround.

(Side note: for that matter, the existing code in RunGen just calls bounds-query once, whereas the code used by infer_input_bounds() does an iterative approach until it finds convergence; that wouldn't necessarily help this situation -- since we're getting an outright error on the first iteration, which should fail for infer_input_bounds() as well -- but would it give us a better inferred input bounds in the long run?)

[abadams]

We have a set of constraints on the input sizes. Some of those are >= constraints from bounds relationships, some are equality constraints set by the user with set_min/set_extent, and some are totally unstructured (add_requirement). A bounds query would ideally return a size that meets all the constraints. Right now we try to satisfy this in an ad-hoc way. First we use the bounds relationships to get a minimum size, then we run it through the equality constraints and assert that it doesn't make it smaller. Boundary conditions really mess things up because the bounds relationships are recursive (the input must be as large as the input), and so are the equality constructs (the input must be the size of the input rounded up/down), so you get a suggested bound that depends on the (nonsense) input size. The jit path tries to iterate this whole until convergence to handle interdependent constraints between dimensions.

It might be worth taking a step back and being more principled about this. The code that runs a bounds query should take the entire set of symbolic constraints as a monolithic entity and emit a piece of code that solves the constraint system or returns an error.

[steven-johnson]

The code that runs a bounds query should take the entire set of symbolic constraints as a monolithic entity and emit a piece of code that solves the constraint system or returns an error.

The code in RunGen that runs bounds query doesn't have access to the symbolic constraints in question; all it has access to is the bounds-query entry point that has been created by normal codegen. Am I misunderstanding your suggestion here?

[abadams]

Sorry, I meant the code inside the compiled pipeline. The code that implements the bounds query, not the code that invokes it.

This is not super actionable though - it would be a largish feature.

[steven-johnson]

Ah, gotcha. Agreed. #4128 is actually an excellent workaround for 90+% of likely cases anyway,