Skip to content

Infer input/output buffer constraints from schedule directives - #3423

Open
steven-johnson wants to merge 9 commits into
mainfrom
srj-schedule-storage
Open

Infer input/output buffer constraints from schedule directives#3423
steven-johnson wants to merge 9 commits into
mainfrom
srj-schedule-storage

Conversation

@steven-johnson

Copy link
Copy Markdown
Contributor

This is still a work-in-progress that isn't ready for review, but I'm opening a PR to get wider feedback on it; it's not ready for a proper code review yet, but a conceptual review would be welcome.

The idea here (from @dsharlet) is that we can use the existing scheduling language to infer the right constraints for input and output buffers, avoiding the need to use dim().set_stride() and such entirely; this is motivated by several things:

  • More compact code, less redundant specification (cf the changes to test/interleave)
  • Ability to specify constraints for Generators that use Input<Func> or Output<Func>; putting schedule-methods shims onto these allows you to conveniently use such a Generator both in 'standalone' mode, or via GeneratorStub (in which case the constraints would simply be ignored for inlined Funcs.)

Theoretically, this would allow us to migrate all code to simply use scheduling directives and ~never need to use the set_stride(), etc methods; in practice, there are some holes that aren't addressable with the existing scheduling directives:

  • There's no way to set stride explicitly to a strange value (eg if you have a buffer that is referring to some array of structs or some such)
  • There's no way to set min without setting extent (eg if you want min=0 but extent aligned to a 16-byte boundary); align_bounds() aligns both min and extent.
  • There's no way to clamp an extent to an upper bound (eg if you want to ensure that a buffer is <= a hardware-specific limit)

Also, the implementation has some ugly bits:

  • We emit user warnings if you attempt to constrain the bounds of an inline Func (since this is meaningless and probably an error); with these changes, it becomes reasonable to set constraints on the inputs or outputs of a Generator, which might be ignored in some situations. Rather than try to finesse this, I've just inserted a flag to allow suppressing these warnings for a given Function, and then ruthlessly set it for Generator inputs and outputs. This is probably ok, but it has a code smell I don't like.
  • There's some handwavy TODO stuff in the Generator code (re: limiting access to certain methods) that needs cleaning up.
  • There are a handful of existing tests and apps that have failures that need investigating.

This is still a work-in-progress that isn't ready for review, but I'm opening a PR to get wider feedback on it; it's not ready for a proper code review yet, but a conceptual review would be welcome.

The idea here (from @dsharlet) is that we can use the existing scheduling language to infer the right constraints for input and output buffers, avoiding the need to use `dim().set_stride()` and such entirely; this is motivated by several things:

- More compact code, less redundant specification (cf the changes to test/interleave)
- Ability to specify constraints for Generators that use `Input<Func>` or `Output<Func>`; putting schedule-methods shims onto these allows you to conveniently use such a Generator both in 'standalone' mode, or via GeneratorStub (in which case the constraints would simply be ignored for inlined Funcs.)

Theoretically, this would allow us to migrate all code to simply use scheduling directives and ~never need to use the set_stride(), etc methods; in practice, there are some holes that aren't addressable with the existing scheduling directives:
- There's no way to set stride explicitly to a strange value (eg if you have a buffer that is referring to some array of structs or some such)
- There's no way to set min without setting extent (eg if you want min=0 but extent aligned to a 16-byte boundary); align_bounds() aligns both min and extent.
- There's no way to clamp an extent to an upper bound (eg if you want to ensure that a buffer is <= a hardware-specific limit)

Also, the implementation has some ugly bits:
- We emit user warnings if you attempt to constrain the bounds of an inline Func (since this is meaningless and probably an error); with these changes, it becomes reasonable to set constraints on the inputs or outputs of a Generator, which might be ignored in some situations. Rather than try to finesse this, I've just inserted a flag to allow suppressing these warnings for a given `Function`, and then ruthlessly set it for Generator inputs and outputs. This is probably ok, but it has a code smell I don't like.
- There's some handwavy TODO stuff in the Generator code (re: limiting access to certain methods) that needs cleaning up.
- There are a handful of existing tests and apps that have failures that need investigating.
@steven-johnson

Copy link
Copy Markdown
Contributor Author

Note that apps/fft fails with this: we (correctly) infer that we should constrain stride[1]=16, stride[2]=256... but then explicitly set stride[2] to zero when calling it, as a way to eliminate call overhead from benchmarking.

steven-johnson added a commit that referenced this pull request Nov 8, 2018
It was awkward to use Buffer for non-planar data; `make_interleaved` supports only a limited case. This adds an arbitrary `reorder` method, and related ctors, that allow for easily making Buffers that have storage ordering that doesn't match the dimension ordering. (This will be used by #3423 to make the results of `Func::realize()` have the proper layout automatically.)

Also, some drive-by cleanup in Buffer to use delegating ctors in some spots.
@steven-johnson

Copy link
Copy Markdown
Contributor Author

This is updated to a point at which all of Halide seems to be working properly (with modest fixes); there are still some concerns about parts of the design (which @dsharletg and I have discussed offline), but I would welcome some initial feedback on this to keep it moving forward.

@abadams

abadams commented Nov 8, 2018

Copy link
Copy Markdown
Member

I'm really torn about this. I haven't been weighing in yet because the parallel APIs definitely feel like a language wart, and so it would be nice to unify them, but they serve different purposes. For user-created buffers, we want to express what is and isn't known about them, and what should be asserted. For internal Funcs, we want to direct the algorithm that determines the storage layout.

For simple bounds these things are similar enough that it seems safe to derive buffer bounds from a schedule bound. For anything else differences start showing up immediately. E.g. bounds can be expressed in terms of the existing size (assert it's clamped, even, a perfect square, etc) in a way that doesn't make sense for directing the storage layout of a Func.

reorder_storage is the thing that concerns me the most. I think it makes no sense on an external buffer. You can't reorder its storage layout - we didn't allocate it. I had to go read the source to figure out what it would do (not a great sign). It looks like it changes which dimension we assume has stride 1, and throws a warning if the strides have been messed with in any other way. This is weird because it means that all args except for the first one to reorder_storage are just ignored. There's no difference between reorder_storage(c, x, y) and reorder_storage(c, y, x). Both just unset the stride constraint on x and apply a stride constraint to c. Unless I'm misunderstanding, any situation in which the difference would be meaningful is something that would throw a warning because the strides have been messed with already.

So my takeaway is that this creates a simplified API for external buffers storage constraints that less verbose but also less expressive compared to the existing API, so we'd still need the existing API, and has some calls that work by analogy to the API that directs a storage layout, and the analogy isn't perfect, so I'm left scratching my head as to what things mean.

@steven-johnson steven-johnson added the skip_buildbots Do not run buildbots on this PR. Must add before opening PR as we scan labels immediately. label May 26, 2020
@alexreinking alexreinking added this to the Eventually milestone Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip_buildbots Do not run buildbots on this PR. Must add before opening PR as we scan labels immediately.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants