Page MenuHomePhabricator

[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
805–806

This section needs an update.

836–837

This section needs an update.

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

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
1512

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
1512

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
1512

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
1512

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
1512

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
1512

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
842

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

864

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
1513

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
864

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
864

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.