Thanks for creating this!

Yeah, should backport this one to master ASAP

Not related to this PR - just curious what's your thoughts about how it might be evolving in the future, are we going to add more and more TensorImpl fields here, e.g. contiguous, channels_last, etc, which are more "internal" properties?

strides will subsume contiguous/channels_last, and will be enough to fully specify the output without getting into non dense tensors. There might be more extensibility here when TensorIterator comes in the mix though.

Or should we consider this class to replace the existing "collection of fields" on TensorImpl?

I'd prefer to keep them decoupled for now, to make it easier to make representational changes in one or the other.

+1, we can capture congruences later if they arise naturally (and pay their way)

When I studied the codegen, templates like this helps me learn the connection between internal functions and the final output; but sometimes I felt seeing a little concrete output example could be more intuitive (the downside is that we need remember to update the example). Maybe including a link to the RFC above structured_func()?

Maybe have subfunc return Optional[str] to fit the pattern of the other compute methods, if there's no semantic difference between empty string and None?

Mooted as we no longer gen declarations.

+1, we can capture congruences later if they arise naturally (and pay their way)

Would an explicit constructor for TensorMeta spare us redundant default initialization?

Yeah, hoping to flush this and other inefficiencies out when I'm ready for benchmarking.

Have a constructor now.

...re the explicit constructor comment above, here e.g. I think an explicit TensorMeta m{sizes, self.options()} would express intent directly, rather than relying on the compiler to be smart about eliding redundant inits (and without really sacrificing readability)

much as I like the brevity of sizes, I think calling the parameter output_sizes might avoid some confusion here

The way you've factored this totally conjures the notion of canned forward and backward utility functions wrapped around an op-specific lambda like upsample_nearest1d_common_check , but sample size of 1 is probably an influence here

I hope there are opportunities for utility functions; need to do more functions to say though.

how long will this need to live?

Also - I know the rfc posits a config-by-convention world where the functional and inplace variants don't need to say anything and the whole setup is triggered by structured: True in the foo.out variant. But I wonder if we won't run into trouble when the naming convention needs to be disrupted for one reason or other. In that world we might want a field that actually specifies the structured op we want to delegate to - delegate: upsample_nearest_1d.out or something.

Among other things, that would give us the opportunity to checksum the out variant being structured: True, as opposed to silently generating code that would result in link errors 20 minutes later or whatever :)

Not 100% sure this pays its way, but it feels like the added expression of intent might be a usability win, as well as loosening up the coupling to the (limits of the) current naming convention.

I don't intend to land the PR with the abstract keyword. Need to do some major pre-surgery to get rid though.

But I wonder if we won't run into trouble when the naming convention needs to be disrupted for one reason or other.

There's no overload naming convention here; matching of signatures is done purely by looking at the type of the function, and the function name (sans overload name). That being said...

In that world we might want a field that actually specifies the structured op we want to delegate to - delegate: upsample_nearest_1d.out or something.

I'm OK with doing this. It is technically redundant information but it gives us an opportunity to give better error messages and makes the intent in YAML more clear. That being said...

opposed to silently generating code that would result in link errors 20 minutes later or whatever :)

I'm pretty sure that in the current formulation if you mismatch the signatures of two functions in the same structured block, you'll get a codegen error (because the dangling signature will fail to find the _out version).

You're absolutely right, the grouping machinery is definitely robust enough to handle all the cases I was thinking about (missing out target, multiple clusters in a single op, overload name shenanigans). So I think the only motivation that remains is the thing about improving errors based on expressed intent. [Edit: ...and readability, in the sense of knowing at a functional or inplace kernel's declaration site whether the generated code will be delegating to a structured kernel or not.]

@bhosmer There are also more serious changes we can make to how we setup native_functions.yaml; for example, we can force the user to group related signatures. This actually hearkens to my original original proposal (https://fb.quip.com/Z7PYApb0uqwR#VBbACAwaTQ4 FB-only sorry), but people seemed allergic to it the first time around.

people seemed allergic to it the first time around.

Do you remember what the counterarguments were? I always thought grouping variants together was a good idea - not just for readability in the YAML but also to spare the intake logic from performing exactly that grouping logic (plus sanity checks etc.) behind the scenes.

Now that structured kernels will be adding an explicit intra-group relationship, having a declaration syntax that puts members together makes even more sense to me.

I guess one possible issue is if different variants support different dispatch keys. Does that ever happen?

For the record, the new version of the patch doesn't group native_functions.yaml, but has structured and structured_delegate to make the structure here more explicit

might be worth listing this in the constraints section above... but also, is it necessary? I could imagine func/in/out triads that were all in the overloaded namespace

In the one example I ported they all have overload names :) This invariant enforces that you "stripped" the overload name using signature() to get at the functional signature.

Ah right - sorry, careless read.

nit: dispatch

But also, I'm not sure what this means in relation to the line below..

basically, there is always a one-to-one correspondence between JIT argument (Argument) and meta argument. This is true because I am going to explicitly not support structured kernels for function signatures who cannot support use_c10_dispatcher: full

nit: # TODO SIGHHHHHHH

I think this is already tested on line 341, no?

BTW, this gives me the opportunity to complain again about how big multiply nested functions are easy to write but hard to read 😬 as I read this PR I'm finding myself using search more than I'd like to find track down things like target - a local or param that's like 2 nesting levels and 250 lines away might as well be a global, in terms of readability.

Right now the code is consistent and disciplined, which moderates the effect. But (sorry, broken record) the worry is the future - I'm pretty sure the code you just rewrote looked something like this when freshly written (mod typing and data modeling, ofc).

Anyway I wouldn't push for a rewrite but I'll take the opportunity to tiresomely repeat that I don't know many tactics with better entropy-avoiding ROI than sticking to flat functions whose dependencies are all explicitly plumbed in as parameters.

@bhosmer here's a suggested restructuring: what if we just make classes to represent the closures in question? Then the closed over variables will be explicitly notated as self.blah and you can see what all the closed over variables are in one place.

Yeah, definitely seems like that would solve the readability/entropy worry of ambient referents equally well. I'm not sure as an idiom it's any less cumbersome to write than non-closure functions with everything explicitly passed in, but I'm definitely interested to see how it turns out, if you're down to give it a spin 😁

We fixed it in #47008

IIUC the dispatch key checks here might kick us back out with [], is that ok downstream?

Err, yes? The whole point of returning [] sometimes is that sometimes you don't have a registration for a given dispatch key

Ah got it - I was reading "unsupported" in the called function as meaning unsupported by structured kernels specifically - forgot that this gets called for literally all dispatch keys. All caught up now 😁

Should it be an error to specify out as structured without declaring a functional variant? (Or not an error but an assert, if it's already been checked upstream)

Don't know if there are callers that need this to be permissive, but e.g. the code path that runs through flatten_pre_group() looks like it might benefit from error checking.

It's checked separately, in validate_unstructured. I didn't put error tests here because I don't have error context available.

nit: function

Data modeling OCD obliges me to note the room for nonsense in having both structured and structured_delegate floating around in the representation 😁

In a perfect world we'd have a single property whose type depended on func.kind, but in the absence of that, there's no great choice (assuming making SchemaKind into an ADT that carries this information directly is either impossible in mypy, or unwanted for some other reason).

In the vein of "tighten the representation and provide derived properties for convenience" here's a half-hearted pitch for

structured: Optional['OperatorName']

and out functions just use '' or whatever and is_structured() just tests is not None. get_structured_delegate() could do the obvious.

Yeah, this choice was driven by what I wanted surface syntax to look like, and then trying to keep NativeFunction class looking similar. What do you think is clearer:

- func: foo.out
  structured: None

- func: foo.Tensor
  structured: foo.out

or

- func: foo.out
  structured: true

- func: foo.Tensor
  structured_delegate: foo.out

#2. I think "structured: None means it's structured" would be hella confusing. (More generally I think it's better in the surface syntax to have the attributes be, like, monomorphic :)

Matching representation to surface syntax is a nice through line - might be worth a one-liner in the comment header for NativeFunction (apologies if it's there already and I missed it)