[CIR][MLIR][LoweringThroughMLIR] CIRUnaryOpLowering on float values. #2032
Conversation
terapines-osc-cir
requested review from
andykaylor,
bcardosolopes,
lanza and
xlauko
as code owners
UnaryOpKind Inc, Dec, Plus and Minus can accept float operands, the lowering should also handle those situations.
terapines-osc-cir
force-pushed
the
cir-to-mlir-unary-op-fix
branch
from
e8517f6 to
f3e89e6
Compare
| auto One = mlir::arith::ConstantOp::create( | ||
| rewriter, op.getLoc(), mlir::IntegerAttr::get(type, 1)); | ||
| rewriter.replaceOpWithNewOp<mlir::arith::SubIOp>(op, type, input, One); | ||
| addImmediate(op, type, input, -1, rewriter); |
There was a problem hiding this comment.
while this is correct, should this emit mlir::arith::SubIOp/mlir::arith::SubFOp instead, since that was the case previously?
There was a problem hiding this comment.
I did this just for keeping the duplicated code minimum. If we use subi, there will be another helper method subImmediate.
Instruction set like RISC-V does not have subi instruction, only addi. so I think this is not a weird choice.
| return rewriter.replaceOpWithNewOp<mlir::arith::AddFOp>(op, type, input, | ||
| imm); | ||
| } | ||
| auto imm = mlir::arith::ConstantOp::create(rewriter, op.getLoc(), |
There was a problem hiding this comment.
should we add an assertion that checks if type is an IntegerType before this? not sure if it's a possible case though.
There was a problem hiding this comment.
That makes sense! I've added some checks here, thank you!
bcardosolopes
left a comment
bcardosolopes
left a comment
There was a problem hiding this comment.
Content looks good, but one missing cleanup needed.
| return nullptr; | ||
| } | ||
|
|
||
| mlir::Operation * |
There was a problem hiding this comment.
please refactor the content of these two functions, they are almost identical. You could either use templates or pass a lambda for the replace part.
There was a problem hiding this comment.
Thanks for the suggestion! Code got refactored to use template.
3 participants
UnaryOpKind Inc, Dec, Plus and Minus can accept float operands, the lowering should also handle those situations.
Before this commit, the compiler crash when it met float operands.