KhronosGroup / NNEF-Docs

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

Prohibiting Variables in fragment body (new in v1.0.1) #16

Closed zoeoz closed 5 years ago

zoeoz commented 5 years ago

Can you elaborate on what this change means for the update operation? For example, if a variable tensor is passed as an argument to a fragment, and if that fragment uses the update operation on the variable argument, is this allowed or is it also prohibited?

gyenesvi commented 5 years ago

The idea is that a fragment should not have side effects and inner defined local variables. So the update should be prohibited in fragments as well, since that means side effects for a fragment. This is currently not spelled out in the spec. Is that what you suggest?

Furthermore, there is a missing check in the current parser: it does not even check if the update receives a variable or not. This needs to be fixed.

zoeoz commented 5 years ago

I think your reasons to prohibit update in fragments makes sense.

Regarding the missing check in the parser, we noticed this too. I'm not sure if it can be fixed by syntax without explicitly introducing a new variable tensor type. In our implementation, we store a meta-tag on all tensors internally, to indicate if they are a variable. This allows the check.

gyenesvi commented 5 years ago

I did not mean to fix it by syntax, just storing that info in the parser and doing that check as you say.

zoeoz commented 5 years ago

Glad to hear. IMO, a syntax change may have been overkill. Meta-tags internal to the parser seem to address the issue very well.

gyenesvi commented 5 years ago

Both the spec and the parser has been updated according to the above.