[mlir][tosa] Remove Tosa MulOp Commutative attribute #163312
Conversation
|
@llvm/pr-subscribers-mlir-tosa @llvm/pr-subscribers-mlir Author: None (ShivaChen) ChangesThe patch motiviates by following cases in Tosa Conformance Test. conformance/operators/ew_binary/mul/mul_21x44_i8_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_44x57_i16_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_52x31_i32_perm0_shift8_dyn Shift operand could be non-constant when dynamic extension enabled. With Commutative attribute, all the operands would be treated as commutative. Shift operand could be reordered with one of the MulOp inputs incorrectly in above cases. Would there have better way to fix the issue? Full diff: https://github.com/llvm/llvm-project/pull/163312.diff 1 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td b/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
index 48759f2a3c9e8..697a04e94441a 100644
--- a/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
+++ b/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.td
@@ -1027,7 +1027,6 @@ def Tosa_MinimumOp : Tosa_ElementwiseOp<"minimum", [
def Tosa_MulOp : Tosa_Op<"mul", [
DeclareOpInterfaceMethods<InferShapedTypeOpInterface,
["inferReturnTypeComponents"]>,
- Commutative,
Pure]> {
let summary = "Multiplication operator.";
|
lhutton1
left a comment
lhutton1
left a comment
There was a problem hiding this comment.
Thanks for the fix @ShivaChen, is it possible to add a test at all?
lhutton1
changed the title
ShivaChen
force-pushed
the
remove-tosa-mul-commutative
branch
from
2af0037 to
497caac
Compare
Hi @lhutton1, I added no_shift_op_reorder to check that tosa-layerwise-constant-fold pass won't reorder the shift operand. |
lhutton1
left a comment
lhutton1
left a comment
There was a problem hiding this comment.
Thanks for adding the test @ShivaChen, just had a small nitpick before merging
|
|
||
| // no_shift_op_reorder checks that %arg1 won't be reorder with %0 | ||
| // by the folder pass. | ||
| func.func @no_shift_op_reorder (%arg0 : tensor<44x1xi16>, %arg1 : tensor<1xi8>) -> tensor<44x57xi32> { |
There was a problem hiding this comment.
nit: missing a CHECK-LABEL line here
The patch motiviates by following cases in Tosa Conformance Test. conformance/operators/ew_binary/mul/mul_21x44_i8_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_44x57_i16_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_52x31_i32_perm0_shift8_dyn Shift operand could be non-constant when dynamic extension enabled. With Commutative attribute, all the operands would be treated as commutative. Shift operand could be reordered with one of the MulOp inputs incorrectly in above cases.
ShivaChen
force-pushed
the
remove-tosa-mul-commutative
branch
from
9dd5283 to
959e89d
Compare
The patch motivates by following cases in Tosa Conformance Test. conformance/operators/ew_binary/mul/mul_21x44_i8_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_44x57_i16_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_52x31_i32_perm0_shift8_dyn Shift operand could be non-constant when dynamic extension enabled. With Commutative attribute, all the operands would be treated as commutative. Shift operand could be reordered with one of the MulOp inputs incorrectly in above cases. Would there have better way to fix the issue?
The patch motivates by following cases in Tosa Conformance Test. conformance/operators/ew_binary/mul/mul_21x44_i8_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_44x57_i16_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_52x31_i32_perm0_shift8_dyn Shift operand could be non-constant when dynamic extension enabled. With Commutative attribute, all the operands would be treated as commutative. Shift operand could be reordered with one of the MulOp inputs incorrectly in above cases. Would there have better way to fix the issue?
3 participants
The patch motiviates by following cases in Tosa Conformance Test. conformance/operators/ew_binary/mul/mul_21x44_i8_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_44x57_i16_perm0_shift0_dyn conformance/operators/ew_binary/mul/mul_52x31_i32_perm0_shift8_dyn
Shift operand could be non-constant when dynamic extension enabled. With Commutative attribute, all the operands would be treated as commutative. Shift operand could be reordered with one of the MulOp inputs incorrectly in above cases.
Would there have better way to fix the issue?