KhronosGroup / NNEF-Docs

NNEF public repository
Apache License 2.0
14 stars 3 forks source link

Casting from primitive types to tensors #13

Closed zoeoz closed 5 years ago

zoeoz commented 5 years ago

When the spec says:

Primitive types can be cast to the tensor types of the corresponding data-type when the tensor type denotes an input of a fragment, but casting from primitive types to tensors is not allowed for outputs of a fragment.

I'm assuming this means if an argument of an invocation is a primitive type, it can be cast to a tensor; but assignment within the fragment body of a primitive type to a fragment result is not allowed.

For example,

fragment foo( input: tensor< scalar >, value: scalar ) -> ( output: tensor< scalar > ) {
    temp = exp( value );  # OK: value is an input argument to exp()
    output = value;  # ERROR: assignment of primitive type to tensor result within fragment body
}

The reason I ask, a fragment could return a primitive type as a result, and this can cast to a tensor because it may become an argument to an invocation:

fragment bar( input: scalar ) -> ( output: scalar ) {
    output = input * input;
}

fragment gloop( input: scalar ) -> ( output: tensor< scalar > ) {
    output = exp( bar( scalar ) );  # OK: nested fragment output is input argument to exp()
}

Have I understood correctly?

BTW, what is the rationale for this rule?

gyenesvi commented 5 years ago

Yes, you understood that correctly. The rationale is that if a non-tensor would be returned from a fragment, that value could reach the top level graph. However, in the main graph, only tensors are allowed on the left hand side of assignments, so it would cause problems.

However, you can always just wrap the returned value into a constant, such as

output = constant(shape = [], value =[value])
zoeoz commented 5 years ago

The spec already says on p. 21

All individual identifiers in the graph body must be of type tensor.

So I'm going to suggest the type casting rule on p. 18

Primitive types can be cast to the tensor types of the corresponding data-type when the tensor type denotes an input of a fragment, but casting from primitive types to tensors is not allowed for outputs of a fragment.

be simply changed to:

Primitive types can be cast to the tensor types of the corresponding data-type.

Rationale: the current type-casting rule complicates the type-casting system and creates unnecessary restrictions. For example,

fragment foo( input: tensor< scalar >, value: scalar ) -> ( output: tensor< scalar > ) {
    # CASE 1
    temp = exp( value );  # Currently OK: value is an input argument to exp()
    # CASE 2
    output = value;  # currently an ERROR, but should be OK
}

In the above example, there's no apparent reason CASE 2 should be an error.

fragment bar( input: scalar ) -> ( output: scalar ) {
    output = input * input;
}
graph gloop( input ) -> ( output ) {
    # CASE 3
    output = bar( 1.0 );  # This is already a semantic ERROR due to spec p. 21
}

In CASE 3, the type checking rule from p. 18 isn't necessary, as the rule from p. 21 already defines output as a tensor type.

Actually, this begs the question: why should CASE 3 be an error at all? If output is implicitly declared as tensor type, assigning the primitive value through an implicit cast would still result in a tensor type, agreeing with the rule on p. 21.

Or, alternatively, its enough to clarify that the rule on p. 21 doesn't actually implicitly declare the graph result output as a tensor type... only that the value assigned to output must be tensor type. In that case, CASE 3 is still an error due to the (clarified) p. 21 rule.

In either situation, whatever the desired outcome, the type casting rule on p. 18 isn't needed.

gyenesvi commented 5 years ago

I understand your argument and it makes sense. I would say that allowing CASE 2 would make sense (in this case the parser should make sure to handle the implicit conversion). However, I would not allow CASE 3, because it explicitly declares the output as non-tensor, and that's almost the same as allowing output = 5.0 in the main graph, which we want to avoid. We wanted the main graph to be parseable in a simple way without the need for such substitutions.

By the way, why would you defined a fragment bar for scalars instead of tensors like above? What is the use case for that? Also, why would you define foo as above, with a value of type scalar instead of letting value be a tensor<scalar>?

zoeoz commented 5 years ago

I think it's reasonable to keep CASE 3 an error.

The main point then is the special type-casting rule on p. 18 isn't needed because the rule on p. 21 already has the desired effect. Although, I would still be sure to clarify the rule on p. 21 as mentioned above, i.e., the graph results are not implicitly declared as tensor type... they must be assigned a tensor type (it is a subtle but important difference).

The bar fragment is meant to illustrate what the NNEF language is capable of. I think the use of such fragments would be mainly to aid in argument composition. As indicated in the spec, tuples and fragment results must not mix attribute types and tensor types, to aid compiler optimizations. For example, a compiler that is smart enough could essentially treat the bar fragment as what is known as a constexpr in C++ (basically, an expression that can be completely evaluated at compile-time).

Along this same line of thinking is a possible reason for designing fragment foo as above, with a value of type scalar instead of type tensor<scalar>, i.e., there may be some compile-time transformation of value that is desired within the fragment before initializing the tensor type returned by the fragment.

However, in the actual foo above, value is assigned directly to output without the use of such an argument-composing function mainly to illustrate that within a FRAGMENT body, assigning a primitive type to a tensor result is the same from a type-casting perspective as assigning a primitive type to a tensor parameter (as you highlight in your post). So, this is a simplified example why CASE 2 shouldn't be considered an error.

Another way to view the whole situation is that fragment results have a type that is explicitly declared in the fragment declaration, just as fragment parameters have an explicit type declaration. So, implicit type-casting of a primitive type to a fragment result of compatible tensor type poses no problem within the fragment body (CASE 2), because the fragment result type is explicitly declared (as is the case with fragment parameters). Result identifiers in the graph declaration, however, have no explicit (or implicit) declaration of type and instead must be assigned a type in the graph body (and the assigned type must be a tensor), hence, e.g., why CASE 3 should still be considered an error. In both cases, the desired behavior is completely specified without any need for the special type-casting rule on p. 18.

gyenesvi commented 5 years ago

Actually, the rule in the graph is that all ids in the graph must be assigned a tensor type, not only the outputs, but otherwise, you are right about the subtle difference. So altogether, you convinced me that the type casting rule on p. 18 can be removed.

I understand the hypothetic use of argument composing fragments, that was their intended use case, I was just wondering if you have an actual example where you use such a fragment. I have recently came to the conclusion that such fragments are typically useful for composing shapes of type integer[] (I can imagine heavy use of such fragments), but are rarely needed for composing scalar arguments, for those, simple (inline) scalar expressions are mostly enough (they don't get so complex that why need to be factored out to fragments). Was just curious about your experience if you have any.

zoeoz commented 5 years ago

We currently have a working implementation of the first two stages of NNEF support described on p. 66 (syntax parsing and semantic validation). Will have more real-world experiences shortly as we complete the last two stages. Right now, I'm looking ahead. I can imagine it will be useful. I think it's good design for the NNEF.

gyenesvi commented 5 years ago

Okay, sure, I understand. Just out of curiosity, can you tell me why you need to implement an NNEF parser on your own? What is your use case for which the C++ and Python parsers provided by Khronos are not sufficient?

zoeoz commented 5 years ago

To fully assimilate into our product, internal architectural designs and coding standards; maximizing the control and flexibility of our production environment that only our own implementation would facilitate.

gyenesvi commented 5 years ago

Based on what you describe, your company seems to represent an interesting use case of the NNEF standard. It would be very useful for the NNEF working group if you could share your usage scenarios with us if that is possible. Furthermore, since you have dug quite deep into the details of NNEF, you could also provide valuable input for future directions. The Khronos Group is open for any company to join and influence the development of standards. Let me know if you are interested in more involvement, we can take this conversation offline, you can find my email on my profile.

gyenesvi commented 5 years ago

Fixed in spec version 1.0.1.

zoeoz commented 5 years ago

Hi Viktor,

Doing a diff on the Type Casting section of the new 1.0.1 spec to the previous version, I don't see any change. Did I miss something?

gyenesvi commented 5 years ago

You are right, I have fixed this in the parser code itself, but forgot to remove that half sentence on p 18.

gyenesvi commented 5 years ago

That half sentence has been removed from the spec.