This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ods] Support string literals in `custom` directives
ClosedPublic

Authored by Mogball on Aug 10 2022, 11:00 AM.

Details

Summary

This patch adds support for string literals as custom directive
arguments. This can be useful for re-using custom parsers and printers
when arguments have a known value. For example:

ParseResult parseTypedAttr(AsmParser &parser, Attribute &attr, Type type) {
  return parser.parseAttribute(attr, type);
}

void printTypedAttr(AsmPrinter &printer, Attribute attr, Type type) {
  return parser.printAttributeWithoutType(attr);
}

And in TableGen:

def FooOp : ... {
  let arguments = (ins AnyAttr:$a);
  let assemblyFormat = [{ custom<TypedAttr>($a, "$_builder.getI1Type()")
                          attr-dict }];
}

def BarOp : ... {
  let arguments = (ins AnyAttr:$a);
  let assemblyFormat = [{ custom<TypedAttr>($a, "$_builder.getIndexType()")
                          attr-dict }];
}

Instead of writing two separate sets of custom parsers and printers.

Diff Detail

Event Timeline

Mogball created this revision.Aug 10 2022, 11:00 AM
Mogball requested review of this revision.Aug 10 2022, 11:00 AM
Mogball edited the summary of this revision. (Show Details)Aug 10 2022, 11:01 AM
rriddle requested changes to this revision.Aug 10 2022, 11:03 AM

How often does this come up in practice?

This revision now requires changes to proceed.Aug 10 2022, 11:03 AM
Mogball edited the summary of this revision. (Show Details)Aug 10 2022, 11:03 AM

custom directives can proliferate quite quickly when used a lot in op definitions. Eventually, this leads to a lot of duplication between the parsers and printers which often only differ by a line of code or two. Mixing in C++ is just a way to "parameterize" these custom parsers and printers to improve their reuse.

I'm quite concerned on how unsafe/open this is. This is also missing docs.

What do you mean by unsafe? I suppose there's nothing stopping people from trying to refer to whatever local variables they want, but trying this will lead to C++ compiler errors in most cases anyways because of type mismatches with the parser and printers.

Mogball updated this revision to Diff 451592.Aug 10 2022, 11:58 AM

Update operation and attribute/type docs.

Mogball updated this revision to Diff 451658.Aug 10 2022, 3:28 PM

add test case

These seems helpful to cut down on the number of "near variants" of the same custom format. Jeff, it might help to adopt this in tree. One random example I noticed are the ones in ViewLikeInterface:

void mlir::printOperandsOrIntegersOffsetsOrStridesList(OpAsmPrinter &p,
                                                       Operation *op,
                                                       OperandRange values,
                                                       ArrayAttr integers) {
  return printOperandsOrIntegersListImpl<ShapedType::kDynamicStrideOrOffset>(
      p, values, integers);
}

void mlir::printOperandsOrIntegersSizesList(OpAsmPrinter &p, Operation *op,
                                            OperandRange values,
                                            ArrayAttr integers) {
  return printOperandsOrIntegersListImpl<ShapedType::kDynamicSize>(p, values,
                                                                   integers);
}

printOperandsOrIntegersListImpl btw has no need to be a template, it could take the enum as an argument.

Mogball updated this revision to Diff 451938.Aug 11 2022, 11:44 AM

Good idea. I added a in-tree use where you suggested

rriddle accepted this revision.Aug 12 2022, 12:43 PM
rriddle added inline comments.
mlir/lib/Interfaces/ViewLikeInterface.cpp
73–78

Can we put this in a namespace or rename? The name here is a bit too general for something provided by an interface

mlir/test/mlir-tblgen/op-format.td
19

Can you add a test with escaped characters? e.g. an inner string or something?

This revision is now accepted and ready to land.Aug 12 2022, 12:43 PM
Mogball updated this revision to Diff 452340.Aug 12 2022, 5:54 PM
Mogball marked an inline comment as done.

review comments

Mogball marked an inline comment as done.Aug 12 2022, 5:54 PM
This revision was landed with ongoing or failed builds.Aug 12 2022, 5:55 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Aug 16 2022, 5:54 AM
mlir/lib/Interfaces/ViewLikeInterface.cpp
167

I would prefer if we keep in two different revision the introduction of the ODS feature and the update of the codebase: changes to this file weren't necessary to be coupled I believe.
It makes it a bit harder to track changes through the commit list ("why is my existing custom<OperandsOrIntegersOffsetsOrStridesList> broken by a commit titled [mlir][ods] Support string literals in custom` directives`? Why when I git blame changes I end up with this huge diff?).