Conversation
uenoku
force-pushed
the
dev/hidetou/evaluator-folder
branch
from
b9313bd to
839ba80
Compare
uenoku
force-pushed
the
dev/hidetou/evaluator-folder
branch
from
839ba80 to
50bade4
Compare
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
LGTM
Only nits.
Switching to the folders here makes a ton of sense, like how we can reuse folders in FIRRTL IMCP. Nice idea.
| SmallVector<mlir::OpFoldResult, 1> results; | ||
| om::IntegerAttr resultAttr; | ||
| auto foldResult = op->fold(operandAttrs, results); | ||
| if (failed(foldResult) || results.size() != 1 || |
There was a problem hiding this comment.
I forget how this works. If the folder doesn't apply, then that returns {}. That won't trigger the failure here, right?
There was a problem hiding this comment.
If folder doesn't apply, it returns LogicalResult::failure so it returns an error below (results will be empty)
| !(resultAttr = llvm::dyn_cast_or_null<om::IntegerAttr>( | ||
| results[0].dyn_cast<Attribute>()))) |
There was a problem hiding this comment.
Are these checks necessary/reachable given the constraints of the fold function?
There was a problem hiding this comment.
For the current implementation of folders that implement IntegerBinaryArithmetic, yes it's certainly not reachable.
But I don't think this check is unnecessary since folder could return a value (even when all operands are attributes), and without this check we would get assertion failure or nullptr deference (e.g. string_concat didn't return an attribute when it had single operand without this PR).
This part https://github.com/llvm/circt/pull/10265/changes#r3105827722 is more generic version of this with a slightly better error message.
| // CHECK-DAG: [[WIDEADD:%.+]] = om.constant #om.integer<9 : si6> : !om.integer | ||
| // CHECK: [[DYN:%.+]] = om.integer.add %x, %{{.+}} : !om.integer | ||
| %0 = om.integer.add %i3, %i4 : !om.integer | ||
| %1 = om.integer.mul %i3, %i4 : !om.integer |
There was a problem hiding this comment.
This folding to -4 is right in twos-complement, but may produce unexpected behavior if we're not careful as I think we view OM integers as arbitrary precision. I think the answer to this is just that this is how this works.
2 participants
This extends IntegerBinaryArithmeticOp folder using evaluation rule in Evaluator. Also changes Evaluator to use folder to avoid code duplication. Also extends a folder of StringConcat.
Separated from #10265, which proposes to a pattern to use Op folder not only IntegerArithmeticOp.
Assisted-by: Codex: GPT 5.4