This is an archive of the discontinued LLVM Phabricator instance.

[mlir-tblgen] Support binding multi-results of NativeCodeCall
ClosedPublic

Authored by Chia-hungDuan on May 10 2021, 3:40 AM.

Details

Summary

We are able to bind NativeCodeCall result as binding operation. To make
table-gen have better understanding in the form of helper function,
we need to specify the number of return values in the NativeCodeCall
template. A VoidNativeCodeCall is added for void case.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.May 10 2021, 3:40 AM
Chia-hungDuan requested review of this revision.May 10 2021, 3:40 AM

Nice, thanks, small nits.

mlir/lib/TableGen/Pattern.cpp
339

How about

std::string repl = formatv(...);

? Then you don't need the std::string cast below (same in the other site below which looks identical below - should these always be the same and if so should we use helper?)

447

I think .str() is not needed as you assign directly to a variable of type std::string

mlir/test/lib/Dialect/Test/TestPatterns.cpp
49

How about flipping the order here to ensure that this is called?

mlir/tools/mlir-tblgen/RewriterGen.cpp
948–949

Could you expand here as to what is the incorrect form and how it is captured?

955

This should not be needed as varName is of type std::string

Chia-hungDuan marked 3 inline comments as done.

Addressed the review comments

jpienaar accepted this revision.Jul 11 2021, 6:45 AM

Didn't realize this was still pending another review.

mlir/docs/DeclarativeRewrites.md
439

Mention 0 result one here?

mlir/include/mlir/IR/OpBase.td
2552

I was wondering if void should be at the back, that way we have common prefix, but not sure if better.

mlir/include/mlir/TableGen/Pattern.h
316

This name is a bit confusing. Seems a bit more general than its use. Would argIndexOrSize work?

This revision is now accepted and ready to land.Jul 11 2021, 6:45 AM
Chia-hungDuan marked an inline comment as done.Jul 19 2021, 2:49 AM
Chia-hungDuan added inline comments.
mlir/include/mlir/IR/OpBase.td
2552

After reading it several times, I took your suggestion and switched it to NativeCodeCallVoid.

mlir/include/mlir/TableGen/Pattern.h
316

The other patch(D105677) also changed the same field. I'll merge that first and change this name to DagOrConstant. What do you think? Or the name seems to be to long if we put all the possible meanings?

mlir/lib/TableGen/Pattern.cpp
339

Done. Replaced the auto with std::string(Just tried to keep the same style as other switch-case).

I was thinking about getAllRangeUse, we could support unpacking the return values only, but that means we can't use things like NativeCodeCall<...>:$bind__0 in rewrite pattern. OTOH, for getValueAndRangeUse without indexing, in Kind::Result, it unpacks the values and I just align its semantic. With these two considerations, they just fall into same implementation.

The reason I didn't use a helper for that is, I'm thinking they still mean the different thing. If we want to change the semantic for multiple NativeCodeCall binding, these two cases should be considered separately. I don't to let people think they need to have the same logic here.

BTW, for Kind::Result, they have pretty similar concept as well(in getValueAndRangeUse and getAllRangeUse).

What do you think?

447

The type of getValuePackName() is StringRef and it doesn't seem to have implicit conversion to std::string.

mlir/test/lib/Dialect/Test/TestPatterns.cpp
49

Good idea, done.

Addressed review comment and fix a bug that handleReplaceWithNativeCodeCall
Should be replaced with childNodeNames lookup when preparing arguments for a
call.

Rebase and update the variable name

jpienaar accepted this revision.Jul 20 2021, 7:28 AM
jpienaar added inline comments.
mlir/docs/DeclarativeRewrites.md
444

s/Beside/Additionally/

446

Lets spell this out. if a NativeCodeCall that doesn't return a result, isn't labeled with 0 returns.

But independent of 0, if you report incorrect number of results, then there would probably be OOB accesses that would just fail at runtime. So pretty much, don't do that :)

mlir/include/mlir/IR/OpBase.td
2544

And this is a constraint on the function invoked by NativeCodeCall ?

mlir/include/mlir/TableGen/Pattern.h
256

Nit: add 2 spaces before to make it clearer part of list item

316

SG, if changed in follow up. I think focusing on what it provides in the name would be most important.

mlir/lib/TableGen/Pattern.cpp
339

SG

Chia-hungDuan marked 3 inline comments as done.

Addressed review comment