This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tblgen] Generate generic adaptors for Ops
ClosedPublic

Authored by zero9178 on Dec 25 2022, 5:53 AM.

Details

Summary

This is part of the RFC for a better fold API: https://discourse.llvm.org/t/rfc-a-better-fold-api-using-more-generic-adaptors/67374

This patch implements the generation of generic adaptors through TableGen. These are essentially a generalization of Adaptors, as implemented previously, but instead of indexing into a mlir::ValueRange, they may index into any container, regardless of the element type. This allows the use of the convenient getter methods of Adaptors to be reused on ranges that are the result of some kind of mapping functions of an ops operands.
In the case of the fold API in the RFC, this would be ArrayRef<Attribute>, which is a mapping of the operands to their possibly-constant values.

Implementation wise, some special care was taken to not cause a compile time regression, nor to break any kind of source compatibility.
For that purpose, the current adaptor class was split into three:

  • A generic adaptor base class, within the detail namespace as it is an implementation detail, which implements all APIs independent of the range type used for the operands. This is all the attribute and region related code. Since it is not templated, its implementation does not have to be inline and can be put into the cpp source file
  • The actual generic adaptor, which has a template parameter for the range that should be indexed into for retrieving operands. It implements all the getters for operands, as they are dependent on the range type. It publicly inherits from the generic adaptor base class
  • A class named as adaptors have been named so far, inheriting from the generic adaptor class with mlir::ValueRange as range to index into. It implements the rest of the API, specific to mlir::ValueRange adaptors, which have previously been part of the adaptor. This boils down to a constructor from the Op type as well as the verify function.

The last class having the exact same API surface and name as Adaptors did previously leads to full source compatibility.

Diff Detail

Event Timeline

zero9178 created this revision.Dec 25 2022, 5:53 AM
zero9178 requested review of this revision.Dec 25 2022, 5:53 AM
Mogball added inline comments.Jan 3 2023, 8:19 AM
mlir/lib/TableGen/Class.cpp
284–286
mlir/test/lib/Dialect/Test/TestDialect.cpp
1619

this seems like it should be a unit test

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

please document this parameter in the comment

1297

you don't need to annotate every argument name, typically only those whose value is a simple constant

1308

please spell out this auto

1312

and this one

1344
zero9178 updated this revision to Diff 486235.Jan 4 2023, 4:15 AM

Address review comments:

  • Transform compile test to a unit tests that also checks that accessing operands works properly
  • Fix style issues
zero9178 marked 6 inline comments as done.Jan 4 2023, 4:17 AM
zero9178 added inline comments.
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1297

I tried to find some sort of middle ground where I kept it for those, where the parameter name/meaning is not obvious from the argument passed to it, while removing it from those where it is arguably obvious. Hope this is okay

Overall looks nice

mlir/include/mlir/TableGen/Class.h
515

Could you add what each part means? That is how the params are encoded.

605

I feel this is more an implementation detail for correctness than a user facing item so I'd use a regular comment below rather than doxygen comment in function.

639

Why addMethod vs addMethodAndPrune?

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

I think (but try and check) this will read easier if you hoist out a
bool isGenericAdaptorBase and then use an if-then-else rather than ternary operators inside the function invocation (this will also result in almost all parameters being documented by appropriate variable names (and then you could merge with conditional just below too).

Mmm, do you even use opClass is it is generic adaptor base in this function?

mlir/unittests/IR/AdaptorTest.cpp
25 ↗(On Diff #486235)

Where is a used?

zero9178 updated this revision to Diff 486654.Jan 5 2023, 12:56 PM
zero9178 marked an inline comment as done.

address review comments

zero9178 marked 2 inline comments as done.Jan 5 2023, 1:03 PM
zero9178 added inline comments.
mlir/include/mlir/TableGen/Class.h
515

I wasn't quite sure whether you meant, how the template prameters are "encoded" within the data structure, or whether you mean the template parameters chosen for SetVector (which I've admittedly just copy pasted from elsewhere). I've added a comment elaborating a bit on the former.

639

This just dispatches to the addMethod above, so ends up calling addMethodAndPrune, but also takes into account the additional logic for template parameters.

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

Hoisting the bool definitely made it more readable, but I left it without and if for now, since I managed to reduce the ternary arguments to just the opClass parameter.
I moved the "special case" for the rangeSizeCall to within that functions implementation, where it makes a lot more sense, since that is also where the value used then as rangeSizeCall is defined.

mlir/unittests/IR/AdaptorTest.cpp
25 ↗(On Diff #486235)

I added these more or less as "compile test" sanity checks, to force the compiler to instantiate the templates and catch any non-generic code that may have been generated by TableGen.

That said, they probably do not add a lot of value over the properly exercised ArrayRef<int> instantiations below, so I can remove these if you like

zero9178 marked an inline comment as done.Jan 5 2023, 1:10 PM
zero9178 added inline comments.
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1294

Also, the opClass is used below for generating all the getters for all the operands. This function call is the actual exception where the
proper getODSOperandIndexAndLength is generated in the non-templated base class. Both since it is a bigger bulk of the code, but also because its implementation may use the attribute name for operand_segment_sizes, which forms a circular dependency on the OpClass, hence needing to be in the cpp file (otherwise we'd have to move the adaptor generation below the class, which would be possible too I suppose)

Mogball accepted this revision.Jan 6 2023, 11:31 AM
Mogball added inline comments.
mlir/unittests/IR/AdaptorTest.cpp
64 ↗(On Diff #486654)

newline pls

This revision is now accepted and ready to land.Jan 6 2023, 11:31 AM
zero9178 updated this revision to Diff 486945.Jan 6 2023, 11:42 AM

Address review comments

  • "newline pls" :-)
This revision was automatically updated to reflect the committed changes.