[MLIR][Transform] Add attribute in MatchOp to filter by operand type #67994

pabloantoniom · 2023-10-02T14:49:32Z

This patchs adds the filter_operand_type attribute to transform::MatchOp. With this attribute, the MatchOp will only match an op when the type of all the operands are equal to the specified type.

github-actions · 2023-10-02T15:00:23Z

✅ With the latest revision this PR passed the C/C++ code formatter.

pabloantoniom · 2023-10-03T08:13:40Z

It seems like build failure is related to 39fec54.

pabloantoniom · 2023-10-10T10:35:19Z

Ping

joeyye-work · 2023-11-02T15:42:32Z

(Rewritten) A negative testcase will be nice, say to check if a match fail when operand type does NOT match.

…atchOp

pabloantoniom · 2023-11-07T16:07:36Z

(Rewritten) A negative testcase will be nice, say to check if a match fail when operand type does NOT match.

Thanks for your feedback, I just added a second test case to cover the negative case as well.

matthias-springer · 2023-11-10T02:10:52Z

mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td

@@ -535,6 +535,7 @@ def MatchOp : Op<Transform_Dialect, "structured.match",
      - attribute: the matched op must have all specified attributes (with their
        specified values).
      - filter_result_type: the matched op must return exactly this one type.
+      - filter_operand_type: all the operands of the matched op must must be of this type.


nit: add line break

Solved, thanks for the tip

matthias-springer · 2023-11-10T02:14:08Z

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp

@@ -1180,6 +1180,15 @@ transform::MatchOp::apply(transform::TransformRewriter &rewriter,
        return;
    }

+    if (getFilterOperandType().has_value()) {
+      Type t = getFilterOperandType().value();
+      for (auto type : op->getOperandTypes()) {


nit: Can use !llvm::all_of. Also, drop trivial braces.

Solved, although I personally feel that with llvm::all_of it is less readable

matthias-springer · 2023-11-10T02:15:54Z

mlir/test/Dialect/Linalg/transform-op-match.mlir

+
+// -----
+
+func.func @by_operand_type_negative_match() {


duplicate test case

Solved, thanks

matthias-springer · 2023-11-10T02:17:14Z

mlir/test/Dialect/Linalg/transform-op-match.mlir

+
+// -----
+
+func.func @by_operand_type_negative_match() {


A single test case for "match" and "no match" should be sufficient. E.g., add two ops "test.foo"() : () -> (i32) and "test.foo"() : () -> (f32). One will be matched, the one other one won't.

Solved, thanks for the tip

nicolasvasilache · 2023-11-15T13:41:12Z

mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td

@@ -556,7 +558,8 @@ def MatchOp : Op<Transform_Dialect, "structured.match",
                       OptionalAttr<StrArrayAttr>:$ops,
                       OptionalAttr<MatchInterfaceEnum>:$interface,
                       OptionalAttr<DictionaryAttr>:$op_attrs,
-                       OptionalAttr<TypeAttr>:$filter_result_type);
+                       OptionalAttr<TypeAttr>:$filter_result_type,
+                       OptionalAttr<TypeAttr>:$filter_operand_type);


This seems too restrictive: operations often have multiple operand types, can we turn this into a list ?

Bonus points for doing the same for the result_types (although single result ops are much more common)

Thanks for suggesting this. Actually, I considered doing it, but I didn't because it was unclear to me how the list should work.

After considering it again, I have added a new commit which adds supports for this. The list expects to have a length equal to the number of operands, and the match.op will only succeed if the list of operand types match exactly the operand types in the target op (following the same order).

Besides, I was not sure how to report the error when the length of the list is incorrect, because the error cannot be reported inside the lambda. Thus, I have created an auxiliary variable wrong_operand_filter that is checked after the lambda, but I'm open to change it if there is a more elegant way.

PS: For the result_types I agree, but I feel like it is more appropriate for a separate PR

pabloantoniom · 2023-12-01T11:11:26Z

Ping @matthias-springer @nicolasvasilache

matthias-springer

Sorry for the delay.

matthias-springer · 2023-12-02T01:43:12Z

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp

@@ -1141,6 +1141,7 @@ transform::MatchOp::apply(transform::TransformRewriter &rewriter,
  }

  SmallVector<Operation *> res;
+  bool wrong_operand_filter = false;


camel case: wrongOperandFilter. Nit: Actually, I would call the variable incorrectNumOperandTypes or sth. like that

Solved, thanks

matthias-springer · 2023-12-02T01:43:37Z

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp

+      }
+
+      for (auto const &it :
+           llvm::zip(getFilterOperandTypes().value(), operandTypes)) {


nit: Use llvm::zip_equal

Solved, thanks for the tip

matthias-springer · 2023-12-02T01:45:28Z

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp

+        return;
+      }
+
+      for (auto const &it :


You can write for (auto [attr, operandType] : llvm::zip_equal ... to avoid std::get.

Good to know, thanks

matthias-springer · 2023-12-02T01:46:51Z

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp

+           llvm::zip(getFilterOperandTypes().value(), operandTypes)) {
+        auto attr = dyn_cast<mlir::TypeAttr>(std::get<0>(it));
+        Type type = attr.getValue().cast<::mlir::Type>();
+        Type t = getElementTypeOrSelf(std::get<1>(it));


I think there should be no getElementTypeOrSelf.

It has been removed in the latest commit

matthias-springer · 2023-12-02T01:50:06Z

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp

+    if (getFilterOperandTypes().has_value()) {
+      mlir::ArrayAttr types = getFilterOperandTypes().value();
+      auto operandTypes = op->getOperandTypes();
+      if (types.size() != operandTypes.size()) {


This is different from the op documentation:

all the operands of the matched op must must be of this type

Does not handle the case where an op has multiple operands but only one type is specified in the transform op. (If you did not intend to support such cases, update the documentation. It sounds like if there's 1 type but the op has multiple operands, then all operands must have that type.)

You are right. Actually I want it to support that case so I have fixed the implementation, it should be fixed now

…than one type. Small fixes. Add more testcases

pabloantoniom · 2023-12-04T15:50:52Z

Sorry for the delay.

Thanks for your time. Apart from the fixes, I have added a few more test cases to show different scenarios:

1st test: It works for a single specified type with a target op with a single operand
2nd test: It works for a single specified type with a target op with multiple operands (all operands must be of the specified type)
3th test: It does not match a non-matching type with a target op with a single operand
4th test: It does not match a non-matching type with a target op with multiple operands (one operand matches but the other does not)
5th test: It reports an error when the length of the specified types does not match the number of the operands in the target op

matthias-springer · 2023-12-05T00:00:18Z

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp

+        for (auto [attr, operandType] :
+             llvm::zip_equal(getFilterOperandTypes().value(), operandTypes)) {
+          auto typeattr = dyn_cast<mlir::TypeAttr>(attr);
+          Type type = typeattr.getValue().cast<::mlir::Type>();


Is the cast necessary? Also please write “cast<…>(x)”. the other syntax is deprecated.

Yes, I think it's needed because attr is of type Attribute but we need it to be TypeAttr. I have changed the cast to the one you suggested, thanks for the tip!

nicolasvasilache

thanks!

pabloantoniom · 2023-12-05T09:11:53Z

I don't see the merge button anywhere @nicolasvasilache @matthias-springer, and github says that "Only those with write access to this repository can merge pull requests", so it seems like I don't have access. Can you grant me write access so I can merge it myself? I'd like to contribute more patches in the future. Thanks!

joeyye-work · 2023-12-06T10:24:21Z

I don't see the merge button anywhere @nicolasvasilache @matthias-springer, and github says that "Only those with write access to this repository can merge pull requests", so it seems like I don't have access. Can you grant me write access so I can merge it myself? I'd like to contribute more patches in the future. Thanks!

Please follow https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access to request commit access.

…nd type (#67994)" This reverts commit c439913. Test fails https://lab.llvm.org/buildbot/#/builders/272/builds/2757

metaflow · 2023-12-07T09:30:08Z

test fails https://lab.llvm.org/buildbot/#/builders/272/builds/2757

rikhuijzer · 2023-12-07T10:36:58Z

test fails https://lab.llvm.org/buildbot/#/builders/272/builds/2757

Thanks for the revert. For what it's worth. The test was also failing for me locally. Maybe the cause is that f30a402 tested against an outdated main?

pabloantoniom · 2023-12-07T10:41:50Z

test fails https://lab.llvm.org/buildbot/#/builders/272/builds/2757

Thanks for the revert. @rikhuijzer I think it's because the MLIR tests have changed, and now they require to have a named transform sequence. My bad, I'll fix that shortly.

…nd type (#67994)" Test was failing due to a different transform sequence declaration (transform sequence were used, while now it should be named transform sequence). Test is now fixed.

llvmbot added mlir:linalg mlir labels Oct 2, 2023

[MLIR][Transform] Add attribute in MatchOp to filter by operand type

c3b9f85

pabloantoniom force-pushed the match-operand-types branch from 66efa5f to c3b9f85 Compare October 2, 2023 15:15

This comment was marked as outdated.

Sign in to view

[MLIR][Transform] Add negative test case for filter_operand_type in M…

e34f995

…atchOp

kuhar requested review from matthias-springer and nicolasvasilache November 9, 2023 16:14

matthias-springer reviewed Nov 10, 2023

View reviewed changes

pabloantoniom added 2 commits November 10, 2023 11:27

[MLIR][Transform] Simplify test cases. Use llvm::all_of to filter types

5ba5bcb

[MLIR][Transform] Run clang-format

c867db3

nicolasvasilache reviewed Nov 15, 2023

View reviewed changes

[MLIR][Transform] Convert filter_operand_type into a list of types

5093221

pabloantoniom requested review from nicolasvasilache and matthias-springer November 23, 2023 16:41

matthias-springer reviewed Dec 2, 2023

View reviewed changes

[MLIR][Transform] Fix the behaviour of filter_operand_type with more …

e2545a9

…than one type. Small fixes. Add more testcases

pabloantoniom requested a review from matthias-springer December 4, 2023 15:51

matthias-springer approved these changes Dec 5, 2023

View reviewed changes

nicolasvasilache approved these changes Dec 5, 2023

View reviewed changes

[MLIR][Transform] Replace dyn_cast with cast

f30a402

pabloantoniom merged commit c439913 into llvm:main Dec 7, 2023

metaflow added a commit that referenced this pull request Dec 7, 2023

Revert "[MLIR][Transform] Add attribute in MatchOp to filter by opera…

1087940

…nd type (#67994)" This reverts commit c439913. Test fails https://lab.llvm.org/buildbot/#/builders/272/builds/2757

[MLIR][Transform] Add attribute in MatchOp to filter by operand type #67994

[MLIR][Transform] Add attribute in MatchOp to filter by operand type #67994

Uh oh!

Conversation

pabloantoniom commented Oct 2, 2023

Uh oh!

github-actions bot commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pabloantoniom commented Oct 3, 2023

Uh oh!

pabloantoniom commented Oct 10, 2023

Uh oh!

This comment was marked as outdated.

joeyye-work commented Nov 2, 2023

Uh oh!

pabloantoniom commented Nov 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthias-springer Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabloantoniom commented Dec 1, 2023

Uh oh!

matthias-springer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabloantoniom commented Dec 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasvasilache left a comment

Choose a reason for hiding this comment

Uh oh!

pabloantoniom commented Dec 5, 2023

Uh oh!

joeyye-work commented Dec 6, 2023

Uh oh!

metaflow commented Dec 7, 2023

Uh oh!

github-actions bot commented Oct 2, 2023 •

edited

Loading

matthias-springer Nov 10, 2023 •

edited

Loading