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

Resolve matrix multiply / fully connected / gemm operator issues #24

Closed
ezyang opened this issue Sep 14, 2017 · 4 comments
Closed

Resolve matrix multiply / fully connected / gemm operator issues #24

ezyang opened this issue Sep 14, 2017 · 4 comments

Comments

@ezyang
Copy link
Contributor

ezyang commented Sep 14, 2017

Right now, onnx has a single Dot operator which does both matrix multiply and 1D dot product. I would at least like to split these into two concepts. @zdevito has also proposed that we should exposed a gemm, although @ebarsoum thinks such an operator is too low level.

@ghost
Copy link

ghost commented Sep 22, 2017

While there is a fundamental design tension underlying this specific decision, I think it's best to minimize the amount of functional overlap in ONNX operators. While it is convenient for frontends to have many op choices to map to when exporting (FC, GEMM, Dot), I think the extra amount of effort needed for importers leads to a net loss.

In addition, by pushing on importers and exporters to interop with a smaller set of operators, it encourages stronger pattern matching tools/functionality which is ultimately more flexible and future proof.

For example, introducing GEMM in #47 does not add any expressive power to ONNX since the same functionality can already be represented by Add, Dot, and Transpose with appropriate pattern matching in importers that need it, while adding it as an op requires that all importers and exporters have to reason about it.

@ghost ghost mentioned this issue Sep 22, 2017
@ezyang
Copy link
Contributor Author

ezyang commented Sep 22, 2017

Having GEMM is also helpful for backends as well. Suppose you are backend using CuDNN. You are probably going to have something like GEMM available directly. However, if ONNX only standardizes the unfused version, then every backend has to implement the peephole optimization to convert the atomized representation back into the full one. It's not something that can be implemented once in ONNX, because, after all, there's no way to express the optimized operator in ONNX if you don't standardize the fused one! Conversely, we could implement a "decompose" operation in ONNX, to replace GEMM with simpler ONNX operators, in a way that is universal for all importers. So, for the ecosystem, it seems better to have it.

I think there is a limit to this reasoning, because I agree: we shouldn't be adding fused operators to the spec willy nilly. The argument for fusion makes the most sense when a kernel is particularly performance critical. GEMM (and convolution) seem to fall into this regime.

@ebarsoum
Copy link
Contributor

Regarding " then every backend has to implement the peephole optimization to convert the atomized representation back into the full one", that depend on how we expose RNN in ONNX. I still not sold on GEMM, we have bigger fish to fry (RNN, logic OPs, Loop, training...etc) and I feel we are bloating ONNX. We can easily handle that once we start adding optimization hint to ONNX and function support, so a sub-graph can be replaced with a single backend OP.

@prasanthpul
Copy link
Member

Closing this old issue as Gemm was added: https://github.com/onnx/onnx/blob/master/docs/Operators.md#Gemm

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

No branches or pull requests

3 participants