[PATCH] D86904: [mlir] Support for defining Types in tblgen
John Demme via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 16 12:42:43 PDT 2020
jdd added inline comments.
================
Comment at: mlir/include/mlir/TableGen/CodeGenHelpers.h:41
+public:
+ inline NamespaceEmitter(raw_ostream &os, const Dialect &dialect) : os(os) {
+ if (!dialect)
----------------
rriddle wrote:
> rriddle wrote:
> > nit: Same comments on inline here and throughout.
> Seems like this could take the fully qualified cpp namespace string instead of a dialect.
It could, but everything which uses it gets the namespace from the dialect so this enables maximum code sharing.
================
Comment at: mlir/include/mlir/TableGen/TypeDefGenHelpers.h:2
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
rriddle wrote:
> Do we need this file for this commit? This looks like it is only used for the testing and I'd rather leave parsing/printer utilities separately.
It's not just for testing. The auto-generated parsers/printers use the templates in this file so whenever one is using auto-generated printers/parsers this file has to be #included. The alternative is to emit this file in the generated code, but I prefer this approach so as to not bloat the emit'ed code.
I get that this template-based design decision is likely to be controversial. This is one piece where I'm looking for high-level feedback.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86904/new/
https://reviews.llvm.org/D86904
More information about the llvm-commits
mailing list