Page MenuHomePhabricator

[mlir] ODS: support TableGen dag objects to specify OpBuilder parameters
ClosedPublic

Authored by ftynse on Oct 15 2020, 7:45 AM.

Details

Summary

Historically, custom builder specification in OpBuilder has been accepting the
formal parameter list for the builder method as a raw string containing C++.
While this worked well to connect the signature and the body, this became
problematic when ODS needs to manipulate the parameter list, e.g. to inject
OpBuilder or to trim default values when generating the definition. This has
also become inconsistent with other method declarations, in particular in
interface definitions.

Introduce the possibility to define OpBuilder formal parameters using a
TableGen dag similarly to other methods. Additionally, introduce a mechanism to
declare parameters with default values using an additional class. This
mechanism can be reused in other methods. The string-based builder signature
declaration is deprecated and will be removed after a transition period.

Diff Detail

Event Timeline

ftynse created this revision.Oct 15 2020, 7:45 AM
ftynse requested review of this revision.Oct 15 2020, 7:45 AM

Here's an automatic rewriter to change the syntax - https://reviews.llvm.org/D89474. I will update all in-tree users.

Nice - well I think this means I can stop migrating the other OpBuilder usage as these will be migrated again :)

mlir/include/mlir/IR/OpBase.td
1985

Seems I reverted this in my change (I had changed including to excluding)

1989

I don't think you ever specify here that Type should be a C++ type or expectation wrt namespace (seeing as changes related to that)

1991

s/has/may have/ ?

1996

Why not use ? and detect if unset instead? You can even avoid specifying these in the <> section and inside do

let = ...

so you can avoid needing to introduce another sentinel and reuse the builtin one (you do need to be more careful and verify that it is set before trying to query it then, but avoids the linkage).

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1156

This is unreachable code

1191

This is unreachable code

1219

Micro nit: does std::string(llvm::formatv(...)) result in nicer formatting?

lattner added inline comments.Oct 15 2020, 10:16 AM
mlir/docs/OpDefinitions.md
581

This is really cool functionality, I think this is a big step up over the string based thing.

I'd suggest a slightly different approach to this writing - few people know that the (x thing:$foo) syntax is a tablegen "dag" and the writing may come across as scary for people who don't know this stuff.

I'd suggest you keep the first paragraph, then go into the example. Start with a simple example without default arguments (maybe a convenience constructor that takes an integer for the float attr or something), then show that you can add a default argument to it.

This allows you to build the tower of complexity, and explain (in english prose) the concepts as you introduce them.

Thank you for working on this!

rriddle added inline comments.Oct 15 2020, 11:42 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1195

Seems like you could push this into the else branch above, which remove the need for the conditional.

1209

Is this optional aspect here ever used? Seems like all paths return std::string.

Really great stuff Alex, thanks!

ftynse updated this revision to Diff 298571.Oct 16 2020, 2:40 AM
ftynse marked 10 inline comments as done.

Address comments.

Nice - well I think this means I can stop migrating the other OpBuilder usage as these will be migrated again :)

Yeah, your changes nudged be to clean up and propose mine :)

mlir/docs/OpDefinitions.md
581

Great suggestion! I expanded the explanation. There is some text in the introduction that explains relevant TableGen concepts, but it's quite far away from here.

mlir/include/mlir/IR/OpBase.td
1989

Added a description below.

1996

Great idea, thanks!

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1209

I had it for symmetry with the other case, but as @jpienaar pointed out, it is not necessary in that case either so I removed it.

1219

It does, thanks!

jpienaar accepted this revision.Oct 20 2020, 5:38 PM

Looks good, thanks! An extra failure case for line 1189 would be nice to have.

This revision is now accepted and ready to land.Oct 20 2020, 5:38 PM
ftynse updated this revision to Diff 299611.Oct 21 2020, 2:38 AM

Add a test for Tablegen error messages

An extra failure case for line 1189 would be nice to have.

Yeah, we should also add -verify-diagnostics to Tablegen.

This revision was landed with ongoing or failed builds.Oct 21 2020, 2:43 AM
This revision was automatically updated to reflect the committed changes.

An extra failure case for line 1189 would be nice to have.

Yeah, we should also add -verify-diagnostics to Tablegen.

I don't think that works as easily: most error reporting via print fatal error here, this is different from the opt tool where error is returned and propagated up, so we can verify in the driver. I think only think verify brings additionally is to flag unexpected errors, that FileCheck won't. Is there anything else?

An extra failure case for line 1189 would be nice to have.

Yeah, we should also add -verify-diagnostics to Tablegen.

I don't think that works as easily: most error reporting via print fatal error here, this is different from the opt tool where error is returned and propagated up, so we can verify in the driver. I think only think verify brings additionally is to flag unexpected errors, that FileCheck won't. Is there anything else?

Splitting the input file into chunks without having to have as many RUN lines as the file has different checks.

An extra failure case for line 1189 would be nice to have.

Yeah, we should also add -verify-diagnostics to Tablegen.

I don't think that works as easily: most error reporting via print fatal error here, this is different from the opt tool where error is returned and propagated up, so we can verify in the driver. I think only think verify brings additionally is to flag unexpected errors, that FileCheck won't. Is there anything else?

Splitting the input file into chunks without having to have as many RUN lines as the file has different checks.

That is the --split-input-file option (unrelated but mostly only used with verifying errors as it really doesn't help/obscures else). There were some discussion on general tool (https://lists.llvm.org/pipermail/llvm-dev/2020-July/143378.html) to do that [well one could do a combination of csplit and bash file for this case I think as we are only piping it into filecheck post but it would get creative soon :) ]