Introduce m_Constant() which allows matching a constant operation without forcing the user also to capture the attribute value.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/IR/Matchers.h | ||
---|---|---|
54 | This looks a bit weird to me: it matches _any_ side-effect free operation without operands and with one result. There's nothing specific about it being a _constant_. For example, gpu::ThreadIdOp fits the bill. I think the original constant_op_binder was working correctly because it attempted to constant-fold the operation and check if an attribute (e.g. a compile-time constant) value was produced as a result. This is only possible for actual constants. If you want to avoid capturing the value, you may just extend the constant_op_binder to work around the null pointer in bind_value. |
Unit tests: pass. 61312 tests passed, 0 failed and 736 were skipped.
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Passing-by remark:
Please would it be possible to consistently follow the general trend and prefix patch titles with appropriate [tags]?
This is a MLIR patch, so it would have been awesome to tag it as such.
It makes reading throught llvm-commits, and eventually git log, that much easier.
Introduce [mlir] tag in the commit message. Modify the constant_op_binder class as suggested by Alex. Specifically, introduce a class constructor with zero arguments which sets bind_value to nullptr. In the match method, insert an early exit if we match a constantOp, but we don't need to bind to any attribute value.
This looks to me like something better done by a tool wrapping git log and generating the tag before sending an email.
mlir/include/mlir/IR/Matchers.h | ||
---|---|---|
71 | Seems like this will be prone to subtle failure if we allow matching attribute kinds other than Attribute. I would check the cast(and optionally set 'bind_value') before returning true. Another alternative would be to just have another matcher class(that inherits from this one) that contains a field for the attribute to bind. |
Address River's comment by checking the cast before returning true and optionally binding to bind_value.
mlir/include/mlir/IR/Matchers.h | ||
---|---|---|
73 | Avoid the double return here: if (bind_value) *bind_value = attrT; |
Thanks for the contribution.
Can you:
- join this group to enable CI testing of your revisions.
- As described in https://mlir.llvm.org/getting_started/DeveloperGuide/ Follow the git conventions for writing a commit message, in particular the first line is the “title”, it should be followed by an empty line and an optional description. (this changes landed titled "[mlir] m_Constant()" which isn't descriptive enough).
Thanks!
This looks a bit weird to me: it matches _any_ side-effect free operation without operands and with one result. There's nothing specific about it being a _constant_. For example, gpu::ThreadIdOp fits the bill. I think the original constant_op_binder was working correctly because it attempted to constant-fold the operation and check if an attribute (e.g. a compile-time constant) value was produced as a result. This is only possible for actual constants.
If you want to avoid capturing the value, you may just extend the constant_op_binder to work around the null pointer in bind_value.