Page MenuHomePhabricator

[mlir] Support optional attributes in assembly formats
ClosedPublic

Authored by gysit on Mon, May 18, 2:35 AM.

Details

Summary

This revision adds support for assembly formats with optional attributes. It elides optional attributes that are part of the syntax from the attribute dictionary.

Diff Detail

Event Timeline

gysit created this revision.Mon, May 18, 2:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 18, 2:35 AM
ftynse accepted this revision.Mon, May 18, 5:27 AM
This revision is now accepted and ready to land.Mon, May 18, 5:27 AM
Kayjukh accepted this revision.Mon, May 18, 9:21 AM

LGTM after fixing the use of curly braces.

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

nit: It could be advisable to use some curly braces for such nested statements.

This revision was automatically updated to reflect the committed changes.
ftynse added inline comments.Mon, May 18, 9:41 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
893

MLIR is actually quite adamant on the opposite

Kayjukh added inline comments.Mon, May 18, 10:02 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
893

Interesting. Is there a mention of this in the LLVM coding standards? If not I guess it is mostly up to preference.

ftynse added inline comments.Mon, May 18, 10:26 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
893

It's not in LLVM standards or MLIR fixups to them. In practice, MLIR is aggressive on eliding the braces for single-line one-statement bodies.

Kayjukh added inline comments.Mon, May 18, 10:57 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
893

Sure, for single-line bodies I totally agree that avoiding the braces can make things more readable. What I was pointing out in the first comment was

if (...)
  for (...)
    if (...)
      // ...

which I find less readable and potentially more error-prone than

if (...) {
  for (...) {
    if (...)
      // ...
  }
}
mehdi_amini added inline comments.Mon, May 18, 11:33 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
893

If there is a single statement body inside the if, we'll go without braces in MLIR in general I believe