-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert all remaining Generators to prefer statically-dimensioned Inputs and Output where possible #6620
Conversation
…uts and Output where possible
Input<Buffer<>> input_a{"input_a", 4}; | ||
Input<Buffer<>> input_b{"input_b", 4}; | ||
Output<Buffer<>> output{"output", 4}; | ||
Input<Buffer<void, 4>> input_a{"input_a"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little unfortunate. Is it possible to make Buffer<4> work? I don't think that's doable in c++, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, definitely not. IMHO this is a minor price to pay.
@@ -15,38 +15,38 @@ using Halide::Derivative; | |||
// A model weight is either just an input, or an input and an output | |||
// (the updated weights and the ADAM state) depending on whether we're | |||
// doing inference or training. | |||
template<bool training> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this file, runtime dimensionality seems much cleaner (e.g. there's value in having all the types be the same down below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could selectively revert this one. (I did the conversion by deleting all the old ctors temporarily and then fixing everything.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to selectively reverting this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I revert this, can I ask what you mean by "having all the types be the same down below"? Looking at this again, I don't know which types you mean, and so I'm confused about the specific nature of your concern. (I'm trying to understand why you feel the old approach is cleaner, so I can factor different views into future work.)
Tuesday Morning Review Ping |
&head2_filter, &head2_bias, | ||
&filter1, &bias1}; | ||
|
||
for (Weight *w : weights) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's losing the ability to iterate over the weights here that I was referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be fixed pretty trivially by making backprop()
a virtual method and adding a ModelWeightBase
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just pushed a revision to do this.)
…puts and Output where possible This is the same as #6620, except that it omits autoschedulers/adams2019/cost_model_generator.cpp (which is unusually complex and not yet settled as to whether the changes are welcome). Basically an attempt to land the uncontroversial parts.
No description provided.