This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] [ODS] Allowing attr-dict in custom directive
ClosedPublic

Authored by jdd on Oct 20 2020, 12:56 AM.

Details

Summary

Enhance tblgen's declarative assembly format to allow attr-dict in
custom directives.

Diff Detail

Event Timeline

jdd created this revision.Oct 20 2020, 12:56 AM
jdd requested review of this revision.Oct 20 2020, 12:56 AM
rriddle added inline comments.Oct 21 2020, 6:33 PM
mlir/docs/OpDefinitions.md
752–753

This section needs an update.

783–784

This section needs an update.

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

Can we instead change this such that:

  • attr-dict would pass MutableDictionaryAttr as the parameter
  • we always pass in the operation to the custom directive printer, similarly to the OpAsmPrinter parameter?

?

jdd added inline comments.Oct 22 2020, 1:29 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1483

we always pass in the operation to the custom directive printer, similarly to the OpAsmPrinter parameter?

You mean regardless of the custom directive parameters or just in the attr-dict case? The former is a breaking change. I think you mean the former, which is what I would prefer as well. How much code would it break?

rriddle added inline comments.Oct 22 2020, 1:32 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1483

Regardless, i.e. I think we should do both of those things.

How much code would it break?

Not much given the relative recency of custom directives. That being said, I prefer strongly to not use this as a metric for doing something. I prefer that we do the right thing regardless of whether we have to refactor existing code or not.

jdd added inline comments.Oct 22 2020, 1:35 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1483

I agree. How would we notify users? Do we even have to for breakage this small?

rriddle added inline comments.Oct 22 2020, 1:38 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1483

For really small things like this I usually don't notify users. If you do need to notify in the future, I usually just drop a message in discourse letting people know something big is going to happen and give them a timeline to switch(assuming it's a staged change).

jdd added inline comments.Oct 22 2020, 4:32 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1483

attr-dict would pass MutableDictionaryAttr as the parameter

You mean to the parser, right? Presumably the printer wouldn't be modifying anything.

rriddle added inline comments.Oct 22 2020, 4:35 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1483

I meant the printer here. The "mutable" name is a little weird, but it prevents the need to construct a DictionaryAttr if there aren't any attributes. Though an operation without attributes that implements a custom directive is probably pretty rare, so using DictionaryAttr for the printer is probably fine.

Context: https://github.com/llvm/llvm-project/blob/69efcd03bdb9ee7328dd4e75513ae214c1e23519/mlir/lib/IR/Attributes.cpp#L1359

jdd updated this revision to Diff 300128.Oct 22 2020, 5:47 PM
jdd marked an inline comment as done.
  • Adding test, making suggested changes, fixing breaks
jdd marked 4 inline comments as done.Oct 22 2020, 5:50 PM

Not sure I'm interpreting all your comments correctly but it's certainly closer.

rriddle accepted this revision.Oct 26 2020, 11:41 AM

Please add a comment to the description detailing the fact that the print function of custom directives pass the operation as well.

mlir/docs/OpDefinitions.md
785

You shouldn't pass references to operations here, they are value-typed(i.e. just smart pointers).

812

This wouldn't be a NamedAttrList, I would expect MutableDictionaryAttr here. NamedAttrList is expensive to construct, whereas MutableDictionaryAttr is free.

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

This should be getMutableAttrDict

This revision is now accepted and ready to land.Oct 26 2020, 11:41 AM
jdd marked 3 inline comments as done.Oct 27 2020, 4:24 PM
jdd added inline comments.
mlir/docs/OpDefinitions.md
812

Should it be MutableDictionaryAttr or a reference to it? getMutableAttrDict() returns a reference to the internal attribute dictionary and I don't want to give the impression that one should be mutating the object in the printer. Should it be a const reference?

jdd updated this revision to Diff 301126.Oct 27 2020, 4:26 PM
jdd marked an inline comment as done.
  • Changes from feedback #2
rriddle added inline comments.Oct 27 2020, 4:29 PM
mlir/docs/OpDefinitions.md
812

const MutableDictionaryAttr & SGTM

jdd updated this revision to Diff 301127.Oct 27 2020, 4:43 PM
  • const MutableDictionaryAttr&
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Nov 9 2021, 6:21 PM
mlir/test/lib/Dialect/Test/TestOps.td
1647

There is no test actually exercising this op?

jdd added inline comments.Nov 11 2021, 12:21 PM
mlir/test/lib/Dialect/Test/TestOps.td
1647

It would appear not. At least not as part of this patch. I think I would have thought that if the generated file compiled, that'd be good enough.