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

i32 index fixes #180

Open
wants to merge 3 commits into
base: mlir
Choose a base branch
from
Open

i32 index fixes #180

wants to merge 3 commits into from

Conversation

kurapov-peter
Copy link
Collaborator

These are what I have found while experimenting with different small models. When the type of an index is i32 it doesn't fit into an mlir op and there might be problems with accessing them through the constant op on OV side (I'd seen partial data for example). There are quick fixes for the issues I've seen so far.

SmallVector<int64_t> order(coords.begin(), coords.end());
SmallVector<int64_t> order;
if (ov_order_element_type == ov::element::i64) {
ov::Coordinate coords = const_order->get_coordinate_val();
Copy link
Owner

@slyalin slyalin Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indices in const_order as the second input of Transpose cannot be negative, so you can just use const_order->cast_vector<uint64_t>() and leave negative indices as UB.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I tried that, but got only partial result for some reason. Half of the values were correct, others - zeroes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that you used cast_vector, not get_vector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants