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

Add forwarders for 'name()' method for Funcs, ImageParam #6773

Closed
wants to merge 4 commits into from

Conversation

steven-johnson
Copy link
Contributor

(followup to #6772)

@@ -2334,9 +2337,6 @@ class GeneratorOutputBase : public GIOBase {

// }@

#undef HALIDE_OUTPUT_FORWARD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? Is the macro used later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, drive-by change -- apparently a holdover from way back when there were multiple forwarding macros, just dead code now

@steven-johnson
Copy link
Contributor Author

OK, I obviously didn't even test this locally because I thought it was a no-brainer, silly me

@steven-johnson
Copy link
Contributor Author

Had to rename the inherited GIOBase::name() method (as I did for type() yesterday) to avoid ambiguity with the new forwards, PTAL

@abadams
Copy link
Member

abadams commented May 19, 2022

Hrm, I guess the name of an Output doesn't have to match the name of the Func you assign to it. As things currently stand, does calling name() return the Output name (i.e. gio_name)? Does this change it to return the Func name?

@steven-johnson
Copy link
Contributor Author

Hrm, I guess the name of an Output doesn't have to match the name of the Func you assign to it.

Correct -- in this case we take pains to ensure that the name specified in the metadata matches the gio_name rather than the func name (especially since the latter is often uniquified with $2, etc)

As things currently stand, does calling name() return the Output name (i.e. gio_name)?

Yes.

Does this change it to return the Func name?

Yes. (The goal here being to try to get the Input and Output wrappers to be as close to the underlying object as possible.)

FWIW, though, I didn't find any usage inside Halide tests, apps, etc of code inspecting an Input/Output name -- this change was to satisfy a small bit of code inside Google.

@abadams
Copy link
Member

abadams commented May 19, 2022

That seems surprising. My expectation would have been that if you declare an Output with a name, calling name() on it gives you that name back, and you have to do something like my_output_buffer.func().name() or Func(my_output_buffer).name() to get the name of a Func.

@steven-johnson
Copy link
Contributor Author

My expectation would have been that if you declare an Output with a name, calling name() on it gives you that name back

OK, but keep in mind that the (current) Python generator design has no special name() method on inputs or outputs -- the only such method available is the one on the underlying Func, ImageParam, etc. -- so this change brings the C++ behavior closer to the (proposed) Python design.

Also keep in mind that even in C++, the gio_name of all Inputs and Outputs is 100% fixed at compiletime; there is no way to replace those names via GeneratorParam or whatnot, so having a name() getter is something that would rarely be used.

@steven-johnson
Copy link
Contributor Author

Monday Morning Review Ping

@steven-johnson
Copy link
Contributor Author

I'm just going to drop this PR entirely: the only usage of the GIOBase::name() function that I'm aware of was in one piece of Google code, and we changed it to use a hardcoded name instead (because that's essentially what it was using). I presume that if/when other folks realize that the name() method is no longer public, they will let us know whether it's a problem for them.

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.

None yet

2 participants