Adapt implementation of ONNX operators to code from OnnxToFeld.#511
Adapt implementation of ONNX operators to code from OnnxToFeld.#511kffaxen wants to merge 1 commit intoFeldspar:masterfrom
Conversation
This version of Operators enables code generated by OnnxToFeld to be built and run. Implementation is still incomplete, however.
| -- | Flatten a tensor to a matrix | ||
| onnxFlatten :: Pushy vec => Attrs -> vec a -> Push DIM2 a | ||
| onnxFlatten attrs vec = flatPush (P.fromIntegral $ getAttr attrs aaInt 1 "axis") $ toPush vec | ||
| onnxFlatten_1 :: (Pushy vec, Syntax a) => Attrs -> vec a -> Pull DIM2 a |
There was a problem hiding this comment.
Everything seems to be Pully in this module except this stuff. Why do we need to store a reshape (index transformation) to memory?
There was a problem hiding this comment.
This could in principle be done with a Pull vector and consequently be fuseable. The problem is that the index expressions would be horrible, using division and modulus since the result has fewer dimensions than the argument so that a single integer index must be split into several. We have no optimization in place that eliminates them, so they will typically end up in the innermost loop.
There was a problem hiding this comment.
Would be great to have that kind of information in a comment in the source code.
| onnxFlatten :: Pushy vec => Attrs -> vec a -> Push DIM2 a | ||
| onnxFlatten attrs vec = flatPush (P.fromIntegral $ getAttr attrs aaInt 1 "axis") $ toPush vec | ||
| onnxFlatten_1 :: (Pushy vec, Syntax a) => Attrs -> vec a -> Pull DIM2 a | ||
| onnxFlatten_1 attrs vec = toPull $ store $ flatPush (P.fromIntegral $ getAttr attrs aaInt 1 "axis") $ toPush vec |
There was a problem hiding this comment.
Is idxExp something from class Ix?
| onnxAdd :: (Num a, ShapelyU sh1 sh2) | ||
| => Attrs -> Pull sh1 a -> Pull sh2 a -> Pull (UnionShape sh1 sh2) a | ||
| onnxAdd _ = bcAdd | ||
| onnxAdd_2 :: (Num a, ShapelyU sh1 sh2) |
There was a problem hiding this comment.
It would be nice if the handwritten code is readable and these are binary operators, do we really need the _2 on them? Same question for the _5 on onnxBatchNormalization, _3 on onnxGemm, _1 on onnxFlatten, and so on.
There was a problem hiding this comment.
The arity information is necessary because some ONNX operators can be used with different numbers of arguments. To avoid it, onnxToFeld needs to know about those operators and treat them as special cases, something I have tried to avoid, at least for now.
There was a problem hiding this comment.
Do we get a silent failure if we get the special cases wrong? If not, I would prefer an Operators.hs without extra suffixes and two additional lines (| op elem varargOps = "" and | otherwise = "_" <> show (length args)) in OnnxToFeld.hs.
2 participants
This version of Operators enables code generated by OnnxToFeld to
be built and run. Implementation is still incomplete, however.