[CMSIS-NN] Fixed the case with repeating operands in the QNN binary ops #11732
Conversation
|
cc: @Mousius @grant-arm @manupa-arm for code review. |
Mousius
left a comment
Mousius
left a comment
There was a problem hiding this comment.
I have a few questions @ashutosh-arm, hopefully I've not missed the obvious 😸
| "input": np.random.randint(in_min, high=in_max, size=shape, dtype=dtype), | ||
| } | ||
| output_list = generate_ref_data(orig_mod["main"], inputs) | ||
| compile_and_run( |
There was a problem hiding this comment.
Should we not test that the internal functions only have 1 parameter each?
There was a problem hiding this comment.
I have added a check for this above in L182. Thanks @Mousius.
tests/python/contrib/test_cmsisnn/test_scalar_to_tensor_constant.py
Outdated
Show resolved
Hide resolved
| mod = relay.transform.InferType()(mod) | ||
| mod = ScalarToTensorConstants()(mod) | ||
| new_mod = relay.transform.InferType()(mod) | ||
| assert tvm.ir.structural_equal(mod[global_var].body, new_mod[global_var].body) |
There was a problem hiding this comment.
What is this checking? It appears to just check the body hasn't changed after InferType?
There was a problem hiding this comment.
Before this commit, it was producing a different body in the end due to an error in the code. Since the relay model in this test does not contain any scalar constants, expectation is that the pass should not affect the graph.
There was a problem hiding this comment.
From offline discussion it was decided to leave the check for structural equality in there.
…omposite function Change-Id: I7ffd6074bbbe9020b6efe64d48b80f79714ce8bd
|
Thanks @Mousius for the discussions and code review. Would you like to take a look at it again? |
|
Thanks @ashutosh-arm 😸 |
2 participants
This commit is to fix issues in CMSIS-NN passes
that surface when the same operand is repeated
in QNN binary ops.
cc @areusch