This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add initial support for parsing a declarative operation assembly format
ClosedPublic

Authored by rriddle on Jan 25 2020, 1:05 AM.

Details

Summary

This is the first revision in a series that adds support for declaratively specifying the asm format of an operation. This revision
focuses solely on parsing the format. Future revisions will add support for generating the proper parser/printer, as well as
transitioning the syntax definition of many existing operations.

This was originally proposed here:
https://llvm.discourse.group/t/rfc-declarative-op-assembly-format/340

Diff Detail

Event Timeline

rriddle created this revision.Jan 25 2020, 1:05 AM
rriddle updated this revision to Diff 240363.Jan 25 2020, 1:09 AM

Update header comment.

Unit tests: fail. 62151 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: fail. 62151 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

antiagainst marked an inline comment as done.Jan 27 2020, 3:44 PM

This is super awesome! The code is well written so not many comments from me. Thanks River! One high-level comment: can we add document for this? Doing it as a following commit is fine.

mlir/include/mlir/IR/OpBase.td
1583

format itself is a bit too broad and can be confusing. What about naming it as something like asmForm or assemblyForm? This applies to other places; I see you generally use "declarative op format". I think calling it "declarative op assembly form" is more natural and descriptive. WDYT?

mlir/test/mlir-tblgen/op-format-spec.td
2

Awesome tests!!! :)

3

Accidentally breaking the lines? I'm not sure this is gonna work. :)

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

Nit: Add empty lines to make it more readable?

409

This set is common enough. What about making it a utility function?

rriddle updated this revision to Diff 240722.Jan 27 2020, 4:39 PM
rriddle marked 7 inline comments as done.

Resolve comments

rriddle added inline comments.Jan 27 2020, 4:42 PM
mlir/include/mlir/IR/OpBase.td
1583

SGTM

mlir/test/mlir-tblgen/op-format-spec.td
3

Oops thanks for the catch. I had this split when I was writing the test and forgot to re-add it.

Unit tests: fail. 62151 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jpienaar marked an inline comment as done.Jan 27 2020, 9:28 PM

Nice, thanks

One clang-format failure

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

Could we also add what these means? (and/or add an example)

225–226

Perhaps hoist this above and specify that one could either specify 1) general specification, or 2) a per operand/result type ? Else I have to read one comment further to know when the previous would be empty.

Lets also expand std::vector<Optional<int>> part, it is not immediately obvious if the size of the vector match the number of operands/results or what the optional int represents.

251

Why is keyword spelled out here and in end, but abbreviated elsewhere?

607

Is 'name' the only value referenced inside lambda?

697

With these all within one file, having a simple parsing return using bool & using shortcut logic would read easier (E.g., I'm not really convinced LogicalResult helps much here, I feel LogicalResult obscures the parsing logic and doesn't add much value/verification as it is all self-sufficient in this file).

724

s/*not*/not/ , additional emphasis not needed in these error messages.

766

s/covered/bound/ ? Or defined ?

792–798

You could combine these two and update the comment (verification on a valid format field is below, these just determines if there is an assembly format specified).

rriddle updated this revision to Diff 240929.Jan 28 2020, 10:12 AM
rriddle marked 10 inline comments as done.

Resolve comments

rriddle added inline comments.Jan 28 2020, 10:14 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
251

This is used to denote the range of the keywords in the enum, and I don't want to it to seem as if these are 'start' and 'end' keywords.

607

Yes. I'd rather not add explicit capture to avoid formatting on to a new line. The lambda is simple enough as-is IMO that this isn't necessary.

697

Removing the 'failed' wouldn't make it any cleaner/enable formatting. I prefer LogicalResult to enable the use of success/failure, which is where much of the win for this comes in.

Unit tests: pass. 62197 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

flaub added a subscriber: flaub.Jan 28 2020, 5:13 PM
This revision is now accepted and ready to land.Jan 29 2020, 9:03 PM

(please fix clang-format failure before committing)

antiagainst accepted this revision.Jan 30 2020, 6:21 AM

Awesome, thanks River!

rriddle updated this revision to Diff 241526.Jan 30 2020, 11:30 AM

clang-format fix.

Unit tests: pass. 62323 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision was automatically updated to reflect the committed changes.