Add a new directive either to specify the operands can be matched in either order
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Bunch of minor things, but otherwise LGTM. The nested syntax makes a lot of sense since it's restricted to binary operations.
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
751–752 | "are marked" and "ordering" | |
764 | typo | |
mlir/include/mlir/IR/OpBase.td | ||
2688 | same typo xd | |
mlir/lib/TableGen/Pattern.cpp | ||
145–147 | Should probably add a comment for this one. | |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
726 | formatv with no arguments | |
739 | llvm::raw_string_ostream instead? |
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
764–765 | Can you reword this? I'm having trouble parsing this sentence. | |
mlir/lib/TableGen/Pattern.cpp | ||
808–816 | nit: If one branch has braces, all should have braces. Also, please use braces when the body is larger than a single line/statement. | |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
481 | ||
615–623 | nit: Drop else after continue/return/etc |
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
748 | BTW, I'm wondering if there's a better name for this. It may be a little bit confusing with Commutative trait. (The difference is capitalizing or not) | |
764–765 | Sorry, my bad. Rephrase to Only type operand is supported with `commutative` and note that an operation with `Commutative` trait doesn't mean its operand will be matched commtatively. You need to explicitly specify it. Not sure if it's better. | |
mlir/include/mlir/IR/OpBase.td | ||
2688 | Sorry, my bad... | |
mlir/lib/TableGen/Pattern.cpp | ||
145–147 | I think this is a bug before. It's used to collect the location from each operations in the source pattern. So it should include only operation DagNode. In fact, we don't use directive like returnType in source pattern, thus we didn't see issues before. | |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
739 | Here we want to delay the collection of matched operation thus I use a string for caching it. I think raw_string_ostream is similar. Or do you mean if we use raw_string_ostream, that will be more consistent with os << ... style? |
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
748 | How about either? You don't actually care about commutativity, you are effectively expressing 2 patterns in one by allowing two adjacent trees to be matched independent of their order | |
751 | s/have the commutative property/may be matched in either order/ ? | |
751 | two adjacent ? | |
765 | I'm not sure I'm following "type" here. | |
767 | With renaming of the trait you can drop this part as the confusion will be gone :) | |
mlir/lib/TableGen/Pattern.cpp | ||
145–147 | Can one use returnType or location in source pattern? Or are there other cases which would result in count mismatch beyond replace with value [change looks fine, but good to document expectation here] | |
783–784 | I think this is something not clear above: commutative (or what it will be called) results in "unpacking" from 1 node to 2 (do we verify anywhere that it is only 2? else this count need to consider that) | |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
716 | This makes sense to me [well I'd be concerned about shadowing given the variable names, sometihng like ods_match_ or some such prefix to avoid accidentally shadowing]. | |
726 | Why not? | |
733 | I'm not sure if a user would be able to understand this, could we make it easier to understand? |
Address review comment and changed the directive name to either
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
748 | Sounds Good! | |
765 | Sorry, I want to say an operand which is not an Attribute. I'm thinking that if we should allow commutative attributes. It's doable but not sure if it's necessary(or it's reasonable). | |
767 | True :) Thanks | |
mlir/lib/TableGen/Pattern.cpp | ||
145–147 | No, user can't use returnType/location in source pattern. Here we add either directive which will lead to count mismatch. Comment added | |
783–784 | You're right. Comment updated. PS. I restrict the number of operand of either to be 2(checked somewhere). The reason for that is to make the codegen/verification easier. | |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
716 | changed to "either_oprandX". | |
726 | This is to make the codegen/verification easier. It's doable but so far I'm not sure if we need to support that at beginning. I would like to add that when we heard some cases that need it. | |
733 | Changed to "There's no operation that defines operand {0} of {1}", how about this? |
Looks good thanks
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
752 | either ? (both could be that the constraints have to apply to both sides). This sentence is confusing to me in general and I wonder if we should just skip to the example directly as I find that clear. WDYT? | |
759 | accept | |
763 | s/like/than/ ? | |
765 | Arguments consists of operands and attributes, just saying operand here would do (could you change the above too?) | |
mlir/include/mlir/IR/OpBase.td | ||
2688 | Same as above | |
mlir/include/mlir/TableGen/Pattern.h | ||
189 | s/a return/an/ ? | |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
708 | Do we care if the other args are attributes? | |
756 | Lets use LLVM naming style here :) odsEiherOperand[01] ? |
BTW, I'm wondering if there's a better name for this. It may be a little bit confusing with Commutative trait. (The difference is capitalizing or not)