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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Target/Cpp.h:93
+  /// Return the existing or a new name for a Value.
+  const std::string &getOrCreateName(Value val);
+
----------------
StringRef


================
Comment at: mlir/include/mlir/Target/Cpp.h:103
+  private:
+    using ValMapperScope = llvm::ScopedHashTableScope<Value, std::string>;
+    ValMapperScope mapperScope;
----------------
nit: Why not just inline this type? It has one use.


================
Comment at: mlir/include/mlir/Target/Cpp.h:115
+private:
+  friend struct Scope;
+  using ValMapper = llvm::ScopedHashTable<Value, std::string>;
----------------
I don't think this friend declaration is necessary.


================
Comment at: mlir/include/mlir/Target/Cpp.h:124
+
+  llvm::DenseMap<Dialect *, DialectCppEmitter *> emitters;
+  raw_ostream &os;
----------------
nit: Can we get comments on these variables?


================
Comment at: mlir/include/mlir/Target/Cpp.h:130
+
+class DialectCppEmitter {
+public:
----------------
nit: Can you add documentation to this class?


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


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:10
+#include "mlir/IR/Dialect.h"
+#include "mlir/IR/Operation.h"
+#include "mlir/Target/Cpp.h"
----------------
I think several of these can be trimmed, as they are already included transitively.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:31
+    if (regDialect == nullptr) {
+      llvm::errs() << "Dialect '" << it.first() << "' is not registered\n";
+      return failure();
----------------
op.emitError() for all of these?


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:44
+  for (auto &it : emitters) {
+    llvm::errs() << "Adding: " << it.first() << "\n";
+    if (failed(it.second(allocatedEmitters))) {
----------------
This seems like debug output.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:58
+const std::string &CppEmitter::getOrCreateName(Value val) {
+  if (!mapper.count(val)) {
+    mapper.insert(val, formatv("v{0}", ++valueInScopeCount.top()));
----------------
nit: Remove trivial braces.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:61
+  }
+  auto it = mapper.begin(val);
+  return *it;
----------------
nit: Just return `*mapper.begin()`


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:69
+  auto it = emitters.find(dialect);
+  if (it == emitters.end())
+    return nullptr;
----------------
nit: `return emitters.lookup(dialect);`


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:88
+  if (failed(interleaveCommaWithError(
+          op.getOperands(), os, [&](Value result) -> LogicalResult {
+            if (!hasValueInScope(result))
----------------
nit: Can you hoist the functor out of the call for readability?


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:96
+  for (auto attr : op.getAttrs()) {
+    if (std::find(exclude.begin(), exclude.end(), attr.first.strref()) !=
+        exclude.end())
----------------
nit: llvm::is_contained()


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:99
+      continue;
+    if (op.getNumOperands() > 0)
+      os << ", ";
----------------
This seems incorrect to have inside of the loop.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:129
+    os << ") = ";
+  };
+  return success();
----------------
Remove the extraneous semi-colon.


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


================
Comment at: mlir/lib/Target/Cpp/TranslateToCpp.cpp:211
+const llvm::StringMap<CppEmitter::EmitterRegistryFunction> &
+mlir::getCppEmitterRegistry() {
+  return getMutableCppEmitterRegistry();
----------------
Do we really need to expose this? Seems like this provided by an overload of 'emit' that doesn't have an 'emitters' argument.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCppRegistration.cpp:10
+#include "mlir/IR/Module.h"
+#include "mlir/Support/LogicalResult.h"
+#include "mlir/Target/Cpp.h"
----------------
Some of these includes are unnecessary.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCppRegistration.cpp:24
+class StdEmitter : public DialectCppEmitter {
+public:
+  LogicalResult printAttribute(CppEmitter &emitter, Attribute attr) override;
----------------
Please wrap these in anonymous namespaces.


================
Comment at: mlir/lib/Target/Cpp/TranslateToCppRegistration.cpp:34
+
+LogicalResult printConstantOp(CppEmitter &emitter, ConstantOp constantOp) {
+  auto &os = emitter.ostream();
----------------
These methods should be marked static.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76571