variadic identity #40
Conversation
lib/Dialect/RTL/Ops.cpp
Outdated
| @@ -159,6 +160,7 @@ OpFoldResult ExtractOp::fold(ArrayRef<Attribute> operands) { | |||
|
|
|||
| OpFoldResult AndOp::fold(ArrayRef<Attribute> operands) { | |||
| auto size = inputs().size(); | |||
| assert(size > 0 && "rtl.and should take 1 or more operands"); | |||
There was a problem hiding this comment.
Nit, but this should go into the verification hook for rtl.and. As a separate patch, could you write a testcase (in .mlir file) that is invalid and make sure the verifier catches this? If not, we should add verification support. @amaleewilson may be more familiar with this in MLIR.
lib/Dialect/RTL/Ops.cpp
Outdated
| APInt value; | ||
|
|
||
| // and(..., '1) -> and(...) -- identity | ||
| if (matchPattern(inputs[size - 1], m_RConstant(value)) && |
There was a problem hiding this comment.
"inputs.back()" would be more idiomatic here.
|
|
||
| // and(..., '1) -> and(...) -- identity | ||
| if (matchPattern(inputs[size - 1], m_RConstant(value)) && | ||
| value.isAllOnesValue()) { |
There was a problem hiding this comment.
This should check that size() > 1 to avoid making a zero operand And
There was a problem hiding this comment.
We get here after verifier, that will check (size < 1) and folder, that will fold (size == 1)
So we should not have (size < 2) cases at this point.
Do you want to put an assertion here?
There was a problem hiding this comment.
oh right, if folders are guaranteed to run first, then an assertion would be great.
There was a problem hiding this comment.
added size > 1 assertion
lib/Dialect/RTL/Ops.cpp
Outdated
| auto size = inputs.size(); | ||
| APInt value; | ||
|
|
||
| if (size > 1) { |
There was a problem hiding this comment.
nit, but I'd sink the "size > 1" check down into the match pattern line since it is specific to that check.
There was a problem hiding this comment.
I tend to remove this condition similar to the previous question.
lib/Dialect/RTL/Ops.cpp
Outdated
| /// TODO: mul(a, mul(...)) -> mul(a, ...) -- flatten | ||
| if (matchPattern(inputs()[size - 1], m_RConstant(value)) && | ||
| value.isNullValue()) | ||
| return inputs()[size - 1]; |
|
|
||
| func @mul_identity(%arg0: i11, %arg1: i11) -> i11 { | ||
| %c1_i11 = rtl.constant(1 : i11) : i11 | ||
| %0 = rtl.mul %arg0, %c1_i11, %arg1 : i11 |
There was a problem hiding this comment.
nice. Can you add one of these checks with the constant on the left hand side? The generic canonicalization stuff should move the constant to the right so we should handle that, but we should add one test for this.
| let hasFolder = 1; | ||
| let verifier = [{ |
There was a problem hiding this comment.
Nice, thank you for this.
Please pull it out to an out-of-line method (e.g. see how the other custom verifier hooks are defined in the FIRRTL dialect), and possible a separate patch
There was a problem hiding this comment.
Please also add a failing testcase for this to test/rtl/errors.mlir
There was a problem hiding this comment.
added negative test for zero operands case
| let verifier = [{ | ||
| auto size = inputs().size(); | ||
| if (size < 1) | ||
| return emitOpError("requres 1 or more args"); |
|
|
||
| // and(..., '1) -> and(...) -- identity | ||
| if (matchPattern(inputs[size - 1], m_RConstant(value)) && | ||
| value.isAllOnesValue()) { |
There was a problem hiding this comment.
oh right, if folders are guaranteed to run first, then an assertion would be great.
| let verifier = [{ | ||
| auto size = inputs().size(); | ||
| if (size < 1) | ||
| return emitOpError("requres 1 or more args"); |
There was a problem hiding this comment.
Or better yet, this should be a generic constraint. Can you propose something in mlir/include/mlir/IR/OpBase.td?
There was a problem hiding this comment.
Variadics can have 1 argument, but this case is eliminated by folder
lib/Dialect/RTL/Ops.cpp
Outdated
| auto inputs = op.inputs(); | ||
| auto size = inputs.size(); | ||
|
|
||
| APInt value, value1; |
There was a problem hiding this comment.
value1 not used?
Work towards #33
Added identity transformations for variadic operations (and, or, xor, add, mul)