[PATCH] D76571: [mlir] Start simple C++ backend

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar marked an inline comment as done.
jpienaar added a comment.

Updated, thanks



================
Comment at: mlir/include/mlir/Target/Cpp.h:115
+private:
+  friend struct Scope;
+  using ValMapper = llvm::ScopedHashTable<Value, std::string>;
----------------
rriddle wrote:
> I don't think this friend declaration is necessary.
It is required for valueInScopeCount access (which honestly is more to enable making it simple to reuse variable numbers).


================
Comment at: mlir/include/mlir/Target/Cpp.h:155
+    registerCppEmitter(dialect, [=](CppEmitter::DialectCppEmitters &registry) {
+      if (registry.find(dialect) != registry.end())
+        return failure();
----------------
rriddle wrote:
> The description mentions multiple emitters per dialect, but here you are keying on the dialect namespace.
Updated comment above register function.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:167
+  });
+  if (!trailingSemicolon)
+    return failure();
----------------
rriddle wrote:
> I don't understand why this is a failure case, can you add comments?
Just changed to propagate to fallback behavior.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:211
+const llvm::StringMap<CppEmitter::EmitterRegistryFunction> &
+mlir::getCppEmitterRegistry() {
+  return getMutableCppEmitterRegistry();
----------------
rriddle wrote:
> Do we really need to expose this? Seems like this provided by an overload of 'emit' that doesn't have an 'emitters' argument.
Mostly exposed as I don't want to privilege it, I want the registry to be more a convenience for mlir-translate usage. It is easier to use though, but sort of torn as I was thinking about moving the registry completely out of here and so the static registration being optional is more explicit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76571/new/

https://reviews.llvm.org/D76571