This is an archive of the discontinued LLVM Phabricator instance.

[mlir][quant] Move comments to TableGen statements
ClosedPublic

Authored by rikhuijzer on May 29 2023, 5:28 AM.

Details

Summary

The quant dialect documentation currently mostly empty (https://mlir.llvm.org/docs/Dialects/QuantDialect/).
This patch moves the comments to TableGen statements.
By looking at the generated QuantDialect.md, I've confirmed that each change in this patch will end up in the documentation.

The only thing that I wasn't able to document was the UniformQuantizedType.
I haven't found a way to document a DialectType since most appear to be private in other dialects; suggestions are welcome.

Diff Detail

Event Timeline

rikhuijzer created this revision.May 29 2023, 5:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2023, 5:28 AM
rikhuijzer requested review of this revision.May 29 2023, 5:28 AM

DialectType is a constraint - I think we may want to convert these to be TypeDef and then can have longer definitions - but even before that the doc generator could be improved.

mlir/include/mlir/Dialect/Quant/QuantOps.td
32

Nit: Convert and end with period.

(same below, capital start with period end https://mlir.llvm.org/docs/DefiningDialects/Operations/#operation-documentation )

DialectType is a constraint - I think we may want to convert these to be TypeDef and then can have longer definitions - but even before that the doc generator could be improved.

Thank you for taking a look! I'm unsure what you mean by the last part of your sentence. Do you advice to improve the doc generator as part of this patch or is it more of a statement for the future combined with a doc generator improvement?

mlir/include/mlir/Dialect/Quant/QuantOps.td
32

After looking at the sentence "It should be a one-liner without trailing punctuation.", your comment, and the output of rg 'let summary = ' mlir/, I'm very confused. I see capitalization, trailing punctuation, and no-capitalization in mlir/. Let's ignore for now

On second thought, probably the idea is to automatically capitalize the sentence and add a dot during documentation generation. Maybe also italicize. That would improve documentation readability. Let me know if this is what you meant and I'll try to make a patch

Thanks for the doc improvements! Always welcome :)

I'll let Jacques handle the final approval on this :)

@jpienaar, anything I can do to move this patch forward?

jpienaar accepted this revision.Jun 5 2023, 7:23 AM

On second thought, probably the idea is to automatically capitalize the sentence and add a dot during documentation generation. Maybe also italicize. That would improve documentation readability. Let me know if this is what you meant and I'll try to make a patch

Indeed that would be nice (even if you look at the TOC being generated ... it could be improved).

@jpienaar, anything I can do to move this patch forward?

Looks good, let me know if you need help in landing.

mlir/include/mlir/Dialect/Quant/QuantOps.td
32

There is a style convention which is unfortunately inconsistently followed - we could even have the doc generator have a lint mode to catch these, which would be a nice addition too. And of course if it turns out that that style convention needs to be updated, we could do that too.

This revision is now accepted and ready to land.Jun 5 2023, 7:23 AM

Thanks for the reviews, Jacques! I appreciate it.

Indeed that would be nice (even if you look at the TOC being generated ... it could be improved).

It's on my task list and I'll pick it up soon.

Looks good, let me know if you need help in landing.

Yes. :) Could you land this patch and D152123?

This revision was automatically updated to reflect the committed changes.