Fix stack op sanity failure #606
Conversation
meenakshiramanathan1
requested review from
nvukobratTT,
pilkicTT and
dgolubovicTT
as code owners
meenakshiramanathan1
removed request for
nvukobratTT,
pilkicTT and
dgolubovicTT
meenakshiramanathan1
force-pushed
the
mramanathan/stack_op_sanity
branch
from
be73b89
to
9a7a279
Compare
meenakshiramanathan1
requested review from
nvukobratTT and
pilkicTT
and removed request for
nvukobratTT and
pilkicTT
| @@ -246,11 +246,12 @@ def decompose(type, attr, dc, inputs): | |||
| new_inputs = [] | |||
| for inp in inputs: | |||
| inp_shape = inp.shape.as_list() | |||
| inp_shape.insert(axis, 1) | |||
| new_inp = dc.op("reshape", [inp], (*inp_shape,)) | |||
| insertion_index = axis if axis >= 0 else len(inp_shape) + axis + 1 | |||
There was a problem hiding this comment.
Why do we need to convert to positive indexing?
There was a problem hiding this comment.
With negative axis, insertion is not applied correctly
When axis = -4, output shape is (2, 1, 256, 24, 24) while expected shape from eval function is (1, 2, 256, 24, 24), this is being rectified when axis is being converted to positive
There was a problem hiding this comment.
Can we check it in a bit more detail and see why the negative axis isn't supported? From the FFE perspective, we should support both of them.
Edit:
I see the rest of the details here: #615
forge/test/mlir/test_ops.py
Outdated
| def test_stack(params): | ||
| _, _, dim = params[0][0], params[0][1], params[1] | ||
| if dim == -3 or dim == 1: | ||
| pytest.xfail("Tensor rank is not 4") |
There was a problem hiding this comment.
Is this failing on the FFE side?
There was a problem hiding this comment.
It is failing in run_mlir_compiler stage
log: stack_mlir.log
meenakshiramanathan1
force-pushed
the
mramanathan/stack_op_sanity
branch
2 times, most recently
from
dcc6c23
to
de7880f
Compare
| // check whether volume above and below matching dim is the same | ||
| if ((volume_above(input_shape_vec, i) == volume_above(output_shape_vec, dim)) and | ||
| (volume_below(input_shape_vec, i) == volume_below(output_shape_vec, dim))) | ||
| (volume_below(input_shape_vec, i) == volume_below(output_shape_vec, dim)) and | ||
| (i < output_shape_vec.size())) |
There was a problem hiding this comment.
What does this check prevent from happening?
|
@meenakshiramanathan1 It is not clear to me what is the problem we are solving in second point of the PR, and how we are solving it. Is this a workaround to skip out of bounds test case, or is it general fix? Can you please reference/open the issue and explain what is happening? |
Please refer to #615 |
meenakshiramanathan1
force-pushed
the
mramanathan/stack_op_sanity
branch
3 times, most recently
from
5d7e4ef
to
e93d640
Compare
meenakshiramanathan1
force-pushed
the
mramanathan/stack_op_sanity
branch
from
e93d640
to
f54572b
Compare
|
Let's discuss this one on today's sync. I still have few unknowns that I don't fully grasp at the moment |
|
Opened a new PR #669 related to this |
Fix for #506
Trying to access element outside of dimensions: 4for test cases with dim =-2 and dim =3 in optmised graph pass. It was failing because new_dim was not calculated correctly in can_commute_reshape_through_dim in commute_utils.cpp, hence it was going out of bounds, so a condition was added to skip the out of bounds test case.Full log: stack_sanity.log