Page MenuHomePhabricator

Support NativeCodeCall binding in rewrite pattern.
AcceptedPublic

Authored by Chia-hungDuan on Mon, Apr 19, 2:38 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Mon, Apr 19, 2:38 AM
Chia-hungDuan requested review of this revision.Mon, Apr 19, 2:38 AM
jpienaar accepted this revision.Tue, Apr 20, 12:36 PM

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
2504

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
676

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

This revision is now accepted and ready to land.Tue, Apr 20, 12:36 PM
bondhugula requested changes to this revision.Tue, Apr 20, 8:35 PM
bondhugula added a subscriber: bondhugula.

Wouldn't this need an update to the DRR documentation examples?
https://mlir.llvm.org/docs/DeclarativeRewrites/

This revision now requires changes to proceed.Tue, Apr 20, 8:35 PM
Chia-hungDuan marked 2 inline comments as done.

Applied review comments and updated the document.

Chia-hungDuan added inline comments.Fri, Apr 23, 3:11 AM
mlir/include/mlir/IR/OpBase.td
2504

Yes, updated the description about $_self above to make this more clear.

mlir/lib/TableGen/Pattern.cpp
676

Line 670 is to have an Attribute variable bound with symbol $a
Line 661 and Line 675 are Value variable bound with symbol $c and $b respectively. The difference between them is $b has type constraint on it.

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

Chia-hungDuan added inline comments.Fri, Apr 23, 3:21 AM
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 ?

412

-> ... the names can be put in ...

414–415

These lines appear broken - missing an article or other things punctuation-wise. Can you please rephrase?

mlir/lib/TableGen/Pattern.cpp
660

then block should use braces if the else block does.

bondhugula resigned from this revision.Fri, Apr 23, 4:12 AM
This revision is now accepted and ready to land.Fri, Apr 23, 4:12 AM
Chia-hungDuan marked 3 inline comments as done.

Revised the comments

@bondhugula

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

jpienaar added inline comments.Wed, Apr 28, 4:01 PM
mlir/docs/DeclarativeRewrites.md
395

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.

412–413

I'm not sure what this sentence means.

413

Nit: space before (

415

Is reference actually required?

418–419

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–2501

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.

Chia-hungDuan marked 2 inline comments as done.Fri, Apr 30, 2:54 AM
Chia-hungDuan added inline comments.
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)

412–413

Rephrase to "To carry some return values from helper function"

415

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.

418–419

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

Addressed review comments

Chia-hungDuan marked 2 inline comments as done.Sun, May 2, 3:56 PM
Chia-hungDuan added inline comments.
mlir/docs/DeclarativeRewrites.md
396

One thing to add, for pattern like,

def : Pat<(Op1 (Op2 $arg1), $arg2), ...>

we are generating code like
auto castedOp0 = ::llvm::dyn_cast_or_null<Op1>(op0);
auto *op1 = (*castedOp0.getODSOperands(0).begin()).getDefiningOp();
auto castedOp1 = ::llvm::dyn_cast_or_null<Op2>(op1);

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.

415

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.

Addressed missed/wrong replied review comments

jpienaar accepted this revision.Fri, May 7, 12:59 PM

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.

414

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.

417

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)