onnx-mlir
onnx-mlir copied to clipboard
onnx
Different version of Slice Op
@amanwithfruit I am debugging a model and found the error was in ImportNodeSlice. The size for DenseElementAttribute is assumed to be {1}, while the attribute in the test case has two elements. The bug can be fixed by using the size of the attribute instead of {1} for tensor type in DenseElementAttribute.
However, I noticed that the current ImportNodeSliceOp (by #266) is using an old version (v1): axes is an attribute. In the new versions (v10, v11 and v13) of slice, the default value for optional input axes is specified in the specification. There is no attribute for axes.
For this special case, ImportNodeSlice can check the name of attributes first. If "axes" attribute is not found, the default value can be added according to the specification. The code will be able for handle old and new versions of Slice, though a little bit messy.
Let me page this back into my memory and I'll get started on this. Can you show a failing test case/example I can use to motivate the fix part of this? The other parts I will refer to the spec.
Thanks @amanwithfruit . I can not give you that model directly. I asked the owner to simplify the model so that the buggy part can be shared.
I know that pain 😅 I'm still paging things back in so take your time.
By the way, the code handling axes for SliceOp may be used by other operations, such as ReduceSum, Squeeze and Unsqueezed, all of which changes their axes from attribute to input in the new version. As for the default value, I am thinking whether some support should be added into the table gen. At least, we may define that function in the Op definition using table gen so that the code is more declarative.
By the way, the code handling axes for SliceOp may be used by other operations, such as ReduceSum, Squeeze and Unsqueezed, all of which changes their axes from attribute to input in the new version.
Sure - I'll take a look and see if I can re-use anything that already exists
As for the default value, I am thinking whether some support should be added into the table gen. At least, we may define that function in the Op definition using table gen so that the code is more declarative.
Since they're inputs, I'm not sure what we can do? Unless you're suggesting that we hoist them from ONNX/protobuf inputs to MLIR attrs? Or am I misunderstanding your suggestion?
I am thinking of adding a function to generate the default value in the Op definition, hook it with the optional input with default value, and the constructor invokes that function automatically when the corresponding optional input is missing.
@chentong319 do you have an update on the problematic part of the model? I want to make sure I fix the right thing 😅
for documentation - test case is here: https://github.com/onnx/onnx-mlir/issues/596
@chentong319 can this issue be closed, or is there more to do?
