Page MenuHomePhabricator

[mlir] m_Constant()
ClosedPublic

Authored by chelini on Jan 8 2020, 6:30 AM.

Details

Summary

Introduce m_Constant() which allows matching a constant operation without forcing the user also to capture the attribute value.

Diff Detail

Event Timeline

chelini created this revision.Jan 8 2020, 6:30 AM
ftynse requested changes to this revision.Jan 8 2020, 6:39 AM
ftynse added inline comments.
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.

This revision now requires changes to proceed.Jan 8 2020, 6:39 AM

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.

chelini updated this revision to Diff 236837.Jan 8 2020, 8:48 AM

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.

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.

This looks to me like something better done by a tool wrapping git log and generating the tag before sending an email.

rriddle added inline comments.Jan 8 2020, 10:22 AM
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.

lebedev.ri retitled this revision from m_Constant() to [mlir] m_Constant().Jan 8 2020, 10:35 AM
chelini updated this revision to Diff 236980.Jan 9 2020, 1:46 AM

Address River's comment by checking the cast before returning true and optionally binding to bind_value.

ftynse accepted this revision.Jan 9 2020, 1:57 AM
This revision is now accepted and ready to land.Jan 9 2020, 1:57 AM
rriddle accepted this revision.Jan 9 2020, 10:24 AM
rriddle added inline comments.
mlir/include/mlir/IR/Matchers.h
73

Avoid the double return here:

if (bind_value)

*bind_value = attrT;
chelini updated this revision to Diff 237252.Jan 10 2020, 12:37 AM

Address River's comment by avoiding the double return.

ftynse accepted this revision.Jan 10 2020, 1:44 AM

Could you please commit this patch? I don't have commit permission.

This revision was automatically updated to reflect the committed changes.

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!