Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

steven-johnson
Copy link
Contributor

No description provided.

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"};
Copy link
Member

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?

Copy link
Contributor Author

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>
Copy link
Member

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)

Copy link
Contributor Author

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.)

Copy link
Member

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

Copy link
Contributor Author

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.)

@steven-johnson
Copy link
Contributor Author

Tuesday Morning Review Ping

&head2_filter, &head2_bias,
&filter1, &bias1};

for (Weight *w : weights) {
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.)

steven-johnson added a commit that referenced this pull request Mar 7, 2022
…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.
steven-johnson added a commit that referenced this pull request Mar 7, 2022
…puts and Output where possible (#6641)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants