Page MenuHomePhabricator

[mlir][ODS] Add TypeRef directive in Declarative Assembly Format to allow custom UserDirective parser to receive previously parsed types.
AcceptedPublic

Authored by nicolasvasilache on Sep 16 2020, 10:10 AM.

Details

Summary

Specifying type_ref allows decoupling the specification of the type($foo) from its use in custom UserDirective.

This allows writing custom directives such as like:

let assemblyFormat = [{
       `(` $inputs `:` type($inputs) `)`
       ...
       custom<Foo>(
         type_ref($inputs)
     }];

in which parseFoo can implement special behavior based on the type of inputs without having to parse it at the same time.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2020, 10:10 AM
nicolasvasilache requested review of this revision.Sep 16 2020, 10:10 AM

Finishing the impl and adding unit tests.

nicolasvasilache retitled this revision from [mlir][ODS][WIP] Add TypeRef directive to Declarative Assembly Format to allow custom UserDirective printer / parser to receive existing types. to [mlir][ODS] Add TypeRef directive in Declarative Assembly Format to allow custom UserDirective parser to receive previously parsed types..Sep 17 2020, 5:47 AM
nicolasvasilache edited the summary of this revision. (Show Details)

This looks useful.

mlir/docs/OpDefinitions.md
702

Could you also document what is the C++ type passed to the function that backs this directive? We have a list under "Custom directives" below.

mlir/test/lib/Dialect/Test/TestDialect.cpp
312–313

Types should be passed by-value

mlir/test/mlir-tblgen/op-format-spec-2.td
3–4 ↗(On Diff #292462)

Which parsing, filecheck? Have you tried --split-input-file and placing each def into a separate split.

(I'm also surprised to see filecheck used for checking error messages, we usually do -verify-diagnostics but maybe it's absent from mlir-tblgen).

nicolasvasilache marked 3 inline comments as done.Sep 17 2020, 12:48 PM
nicolasvasilache added inline comments.
mlir/test/mlir-tblgen/op-format-spec-2.td
3–4 ↗(On Diff #292462)

Hmm based on the naming of some of those patterns I figured there seems to be a reordering that happens.
By naming the patterns "properly" I am able to put them in the right position so that the Filecheck agrees.

This is pretty nasty, `--split-input-file`` + `-verify-diagnostics`` sound significantly better.

Orthogonal, future work.

nicolasvasilache marked an inline comment as done.

Update

ftynse accepted this revision.Sep 18 2020, 12:28 AM

By naming the patterns "properly" I am able to put them in the right position so that the Filecheck agrees.

Assuming there is no hashtable somewhere inside...

This revision is now accepted and ready to land.Sep 18 2020, 12:28 AM
rriddle requested changes to this revision.EditedSep 18 2020, 1:30 AM

Thanks Nicolas. I have a few comments on the direction, but OOO right now. Setting as blocking for now of that's alright.

This revision now requires changes to proceed.Sep 18 2020, 1:30 AM
rriddle resigned from this revision.Thu, Oct 1, 11:59 AM

Seems like me being on this review was pointless given that you already submitted it. I'll just go ahead and remove myself.

This revision is now accepted and ready to land.Thu, Oct 1, 11:59 AM

I am deeply sorry @rriddle .. I was working on https://reviews.llvm.org/D87767 which was dependent on this.

I had removed the dependence on the mechanism here and wrote the parser/printer manually:
https://github.com/llvm/llvm-project/blob/9744606614df4ba85a4d546c94b3b5ef9d3a3a96/mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp#L1448

But I confused myself with the stack and ended up submitting a squashed version.

I'll try to revert the part related to this revision, it was not meant to land without discussing with you (and I have additional questions re declarative assembly to allow interleaved operands / types like FuncOp arguments).

Sent out https://reviews.llvm.org/D88698 but I I have a bug in the TestOps.inc.
I'll have to wait till tomorrow to finish the revert.

tools/mlir/test/lib/Dialect/Test/TestOps.cpp.inc:3599:23: error: no member named 'addAttribute' in 'mlir::NamedAttrList'
    result.attributes.addAttribute("attr", attrAttr);    if (optAttrAttr)
    ~~~~~~~~~~~~~~~~~ ^
tools/mlir/test/lib/Dialect/Test/TestOps.cpp.inc:3600:25: error: no member named 'addAttribute' in 'mlir::NamedAttrList'
      result.attributes.addAttribute("optAttr", optAttrAttr);  }
      ~~~~~~~~~~~~~~~~~ ^

If you see offhand what is wrong please comment on the other PR.

rriddle added a comment.EditedThu, Oct 1, 8:05 PM

Sent out https://reviews.llvm.org/D88698 but I I have a bug in the TestOps.inc.
I'll have to wait till tomorrow to finish the revert.

tools/mlir/test/lib/Dialect/Test/TestOps.cpp.inc:3599:23: error: no member named 'addAttribute' in 'mlir::NamedAttrList'
    result.attributes.addAttribute("attr", attrAttr);    if (optAttrAttr)
    ~~~~~~~~~~~~~~~~~ ^
tools/mlir/test/lib/Dialect/Test/TestOps.cpp.inc:3600:25: error: no member named 'addAttribute' in 'mlir::NamedAttrList'
      result.attributes.addAttribute("optAttr", optAttrAttr);  }
      ~~~~~~~~~~~~~~~~~ ^

If you see offhand what is wrong please comment on the other PR.

Ah, don't worry about it. It's not that big of a deal. I'll just add comments here. Apologies for delaying in review, dealing with other things.

Looks great Nicolas. Main comment is that it seems like it would be better to just have a general ref instead. Would likely simplify a bit of the implementation here.

mlir/docs/OpDefinitions.md
702

Would it be better instead to have a general ref directive that allows for capturing other things? Like regions, operands, etc.?

mlir/tools/mlir-tblgen/OpFormatGen.cpp
822

Do we need to generate storage for these here? Or can we do it inside of the custom directive instead?

2822

If we use a general ref instead, that would help remove some of this awkwardness.