We are able to bind the result from native function while rewriting
pattern. In matching pattern, if we want to get some values back, we can
do that by passing parameter as return value placeholder. Beside, add
the semantic of '$_self' in NativeCodeCall while matching, it'll be the
operation that defines certain operand.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Is there perhaps some sensible structure that could be verified from the emitted C++ code? Not a change test but something capturing the ordering, matching & rewrite?
mlir/include/mlir/IR/OpBase.td | ||
---|---|---|
2505 | Just to verify: the below is now consistent with this and https://mlir.llvm.org/docs/DeclarativeRewrites/ seems to also be up to date (although the sections both are more rewrite based. | |
mlir/lib/TableGen/Pattern.cpp | ||
675 | What is difference between this and line 670? | |
mlir/test/lib/Dialect/Test/TestOps.td | ||
851 | OOC what happens if this was def : Pat<(SomeBinaryOp (WriteReturnValueToArgument $ret)), $ret), ... > ? (I'm assuming the equality checking would kick in but not sure, and esp if reversed) | |
mlir/test/lib/Dialect/Test/TestPatterns.cpp | ||
38 | Would a Value& also work? | |
38 | "write" is a bit confusing to me, how about getFirstResult? | |
39 | Lets create a negative result too. E.g., if op has even number of results return false, so that we can check the filtering. | |
mlir/test/mlir-tblgen/pattern.mlir | ||
94 | Nit: capture this one too | |
mlir/test/mlir-tblgen/rewriter-errors.td | ||
28 | And this would be (effectively) $_self ? (well if the operation had single result) | |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
282 | operand |
Wouldn't this need an update to the DRR documentation examples?
https://mlir.llvm.org/docs/DeclarativeRewrites/
mlir/include/mlir/IR/OpBase.td | ||
---|---|---|
2505 | Yes, updated the description about $_self above to make this more clear. | |
mlir/lib/TableGen/Pattern.cpp | ||
675 | Line 670 is to have an Attribute variable bound with symbol $a | |
mlir/test/lib/Dialect/Test/TestPatterns.cpp | ||
38 | We can do that. the reason I use pointer type is just to align with ConstantLikeMatcher's behavior. Do you think using reference will be better? | |
39 | Done. Added another case which will fail rewriting. | |
mlir/test/mlir-tblgen/rewriter-errors.td | ||
28 | Yes, the difference is if we want to use the $_self later, we should set it up in $val in this case |
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
409–410 | This is not supported now and I removed it. Actually we can do something like NativeCodeCall<"cast<ArrayAttr>($0)[" # n # "]"> to get the same function. If we want to make simpler to use, I can do it in the later CL. Feel free to let me know. |
A few minor comments related the documentation, etc. I'll defer to @jpienaar to make a pass on the doc addition.
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
395 | Nit: in a source pattern ? | |
413 | -> ... the names can be put in ... | |
415–416 | These lines appear broken - missing an article or other things punctuation-wise. Can you please rephrase? | |
mlir/lib/TableGen/Pattern.cpp | ||
659 | then block should use braces if the else block does. |
Thanks for reviewing the sentences and sorry for my bad writing. Can you take a look the updated one and see if it's more clear now?
Thanks
Chia-hung
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
395–396 | Lets keep the style consistent here $_self will be replaced by the defining operation in a source pattern. | |
396 | Why the defining operation vs the Value matched? Operands are Values rather than Operations. | |
413–414 | I'm not sure what this sentence means. | |
414 | Nit: space before ( | |
416 | Is reference actually required? | |
419–420 | Could you phrase this more directly? Names with attribute constraints will be captured as Attributes while everything else will be treated as Value. ? | |
mlir/include/mlir/IR/OpBase.td | ||
2499–2502 | If you update the above, update this too to match | |
mlir/test/lib/Dialect/Test/TestPatterns.cpp | ||
38 | I think so. At least this isn't a hard requirement. And makes it simpler to write ODS side I feel. |
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
396 | This is defined by the implementation of tblgen, we generate code like, auto *op1 = (*castedOp0.getODSOperands(0).begin()).getDefiningOp(); I have a concern about this, not all Values have a defining operation, e.g., function arguments, right? I'm not sure if we had any reason to generate this pattern. Do you think we need to change it to Value? (This may slightly impact people who use NativeCodeCall in source pattern) | |
413–414 | Rephrase to "To carry some return values from helper function" | |
416 | Yes, this is also defined by implementation. I'm thinking maybe passing by reference by default will be better, but leave it to the user will be easier for tblgen(not complicate to change though). I'm thinking to keep the same logic as mush as possible so I didn't change this one and the above one. | |
419–420 | Thanks, this is much better! | |
mlir/test/lib/Dialect/Test/TestOps.td | ||
851 | Yes, we will get the message like "symbol ret bound more than once" because it causes ambiguity |
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
396 | One thing to add, for pattern like, def : Pat<(Op1 (Op2 $arg1), $arg2), ...> we are generating code like So match the operand is matching on the defining op, it's not bound to the NativeCodeCall only. it applies to all the nested structure. | |
416 | Please ignore the above reply, I messed different things together. It shouldn't be 'reference'. I would like to say either pass-by-reference or pass-by-pointer should be fine. Fixed. |
Thanks!
mlir/docs/DeclarativeRewrites.md | ||
---|---|---|
396 | I think it is actually always on defining op at the moment - as these correspond to nodes of the dag pattern. I think we will probably need to get back to this indeed. So for now this seems accurate. | |
415 | s/can/must/ ? (part of this is just C++, this is only way to set variable passed in) must be either passed by reference or a pointer to variable used as argument so that the matched value can be returned. | |
418 | argument | |
mlir/test/lib/Dialect/Test/TestOps.td | ||
851 | It would be good to file a bug for this, we do allow (Foo $ret, $ret) to mean match Foo, bind $ret to first arg and verify that matches second arg | |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
282 | What does "of certain operand" mean here? Do we not need $_self if no operands are referenced? (else I'd just remove the end) |
Nit: in a source pattern ?