[mlir][EmitC] Refactor MLIR Translate To Cpp #84973
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mlir-emitc @llvm/pr-subscribers-mlir Author: Weizhe (Jack) Chen (weizchen) ChangesThis refactors the MLIR TranslateToCpp part by introducing CppTranslationInterface Patch is 104.50 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84973.diff 23 Files Affected:
diff --git a/mlir/include/mlir/Target/Cpp/CppEmitter.h b/mlir/include/mlir/Target/Cpp/CppEmitter.h
index 30d3fff9fca88b..4aa551fa205123 100644
--- a/mlir/include/mlir/Target/Cpp/CppEmitter.h
+++ b/mlir/include/mlir/Target/Cpp/CppEmitter.h
@@ -14,13 +14,8 @@
#define MLIR_TARGET_CPP_CPPEMITTER_H
#include "mlir/IR/BuiltinTypes.h"
-#include "mlir/IR/Value.h"
-#include "llvm/ADT/ScopedHashTable.h"
-#include "llvm/Support/raw_ostream.h"
-#include <stack>
namespace mlir {
-namespace emitc {
/// Translates the given operation to C++ code. The operation or operations in
/// the region of 'op' need almost all be in EmitC dialect. The parameter
@@ -28,7 +23,6 @@ namespace emitc {
/// arguments are declared at the beginning of the function.
LogicalResult translateToCpp(Operation *op, raw_ostream &os,
bool declareVariablesAtTop = false);
-} // namespace emitc
} // namespace mlir
#endif // MLIR_TARGET_CPP_CPPEMITTER_H
diff --git a/mlir/include/mlir/Target/Cpp/CppTranslationInterface.h b/mlir/include/mlir/Target/Cpp/CppTranslationInterface.h
new file mode 100644
index 00000000000000..d0a964a82ec2ec
--- /dev/null
+++ b/mlir/include/mlir/Target/Cpp/CppTranslationInterface.h
@@ -0,0 +1,55 @@
+//===--- CppTranslationInterface.h - Translation to Cpp iface ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This header file defines dialect interfaces for translation to Cpp.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TARGET_CPP_CPPTRANSLATIONINTERFACE_H
+#define MLIR_TARGET_CPP_CPPTRANSLATIONINTERFACE_H
+
+#include "mlir/IR/DialectInterface.h"
+#include "mlir/Support/LogicalResult.h"
+
+namespace mlir {
+struct CppEmitter;
+
+/// Base class for dialect interfaces providing translation to Cpp. Dialects
+/// should implement this interface with supported operation translations to
+/// be registered and used with translate-to-cpp.
+class CppTranslationDialectInterface
+ : public DialectInterface::Base<CppTranslationDialectInterface> {
+public:
+ CppTranslationDialectInterface(Dialect *dialect) : Base(dialect) {}
+
+ /// Hook for derived dialect interface to provide op translation to Cpp.
+ virtual LogicalResult emitOperation(Operation *op, CppEmitter &cppEmitter,
+ bool trailingSemicolon) const {
+ return failure();
+ }
+};
+
+/// Interface collection for translation to Cpp, dispatches to a concrete
+/// interface implementation based on the dialect to which the given op belongs.
+class CppTranslationInterface
+ : public DialectInterfaceCollection<CppTranslationDialectInterface> {
+public:
+ using Base::Base;
+
+ /// Translates the given operation to Cpp using the derived dialect interface.
+ virtual LogicalResult emitOperation(Operation *op, CppEmitter &cppEmitter,
+ bool trailingSemicolon) const {
+ if (const CppTranslationDialectInterface *iface = getInterfaceFor(op))
+ return iface->emitOperation(op, cppEmitter, trailingSemicolon);
+ return failure();
+ }
+};
+
+} // namespace mlir
+
+#endif // MLIR_TARGET_CPP_CPPTRANSLATIONINTERFACE_H
diff --git a/mlir/include/mlir/Target/Cpp/CppTranslationUtils.h b/mlir/include/mlir/Target/Cpp/CppTranslationUtils.h
new file mode 100644
index 00000000000000..4ca777fa901618
--- /dev/null
+++ b/mlir/include/mlir/Target/Cpp/CppTranslationUtils.h
@@ -0,0 +1,86 @@
+//===- CppTranslationUtils.h - Helpers to used in C++ emitter ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines common helper functions used across different dialects
+// during the Cpp translation.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TARGET_CPP_CPPTRANSLATIONUTILS_H
+#define MLIR_TARGET_CPP_CPPTRANSLATIONUTILS_H
+
+#include "mlir/Dialect/EmitC/IR/EmitC.h"
+#include "mlir/Target/Cpp/TranslateToCpp.h"
+
+using namespace mlir;
+
+/// Convenience functions to produce interleaved output with functions returning
+/// a LogicalResult. This is different than those in STLExtras as functions used
+/// on each element doesn't return a string.
+template <typename ForwardIterator, typename UnaryFunctor,
+ typename NullaryFunctor>
+inline LogicalResult
+interleaveWithError(ForwardIterator begin, ForwardIterator end,
+ UnaryFunctor eachFn, NullaryFunctor betweenFn) {
+ if (begin == end)
+ return success();
+ if (failed(eachFn(*begin)))
+ return failure();
+ ++begin;
+ for (; begin != end; ++begin) {
+ betweenFn();
+ if (failed(eachFn(*begin)))
+ return failure();
+ }
+ return success();
+}
+
+template <typename Container, typename UnaryFunctor, typename NullaryFunctor>
+inline LogicalResult interleaveWithError(const Container &c,
+ UnaryFunctor eachFn,
+ NullaryFunctor betweenFn) {
+ return interleaveWithError(c.begin(), c.end(), eachFn, betweenFn);
+}
+
+template <typename Container, typename UnaryFunctor>
+inline LogicalResult interleaveCommaWithError(const Container &c,
+ raw_ostream &os,
+ UnaryFunctor eachFn) {
+ return interleaveWithError(c.begin(), c.end(), eachFn, [&]() { os << ", "; });
+}
+
+
+/// Determine whether expression \p expressionOp should be emitted inline, i.e.
+/// as part of its user. This function recommends inlining of any expressions
+/// that can be inlined unless it is used by another expression, under the
+/// assumption that any expression fusion/re-materialization was taken care of
+/// by transformations run by the backend.
+bool shouldBeInlined(emitc::ExpressionOp expressionOp);
+
+LogicalResult printConstantOp(CppEmitter &emitter, Operation *operation,
+ Attribute value);
+
+LogicalResult printBinaryOperation(CppEmitter &emitter, Operation *operation,
+ StringRef binaryOperator);
+
+LogicalResult printUnaryOperation(CppEmitter &emitter, Operation *operation,
+ StringRef unaryOperator);
+
+LogicalResult printCallOperation(CppEmitter &emitter, Operation *callOp,
+ StringRef callee);
+
+LogicalResult printFunctionArgs(CppEmitter &emitter, Operation *functionOp,
+ ArrayRef<Type> arguments);
+
+LogicalResult printFunctionArgs(CppEmitter &emitter, Operation *functionOp,
+ Region::BlockArgListType arguments);
+
+LogicalResult printFunctionBody(CppEmitter &emitter, Operation *functionOp,
+ Region::BlockListType &blocks);
+
+#endif // MLIR_TARGET_CPP_CPPTRANSLATIONUTILS_H
diff --git a/mlir/include/mlir/Target/Cpp/Dialect/All.h b/mlir/include/mlir/Target/Cpp/Dialect/All.h
new file mode 100644
index 00000000000000..556588efb0118c
--- /dev/null
+++ b/mlir/include/mlir/Target/Cpp/Dialect/All.h
@@ -0,0 +1,35 @@
+//===----- All.h - MLIR To Cpp Translation Registration ---------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines a helper to register the all dialect translations to Cpp.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TARGET_CPP_DIALECT_ALL_H
+#define MLIR_TARGET_CPP_DIALECT_ALL_H
+
+#include "mlir/Target/Cpp/Dialect/Builtin/BuiltinToCppTranslation.h"
+#include "mlir/Target/Cpp/Dialect/ControlFlow/ControlFlowToCppTranslation.h"
+#include "mlir/Target/Cpp/Dialect/EmitC/EmitCToCppTranslation.h"
+#include "mlir/Target/Cpp/Dialect/Func/FuncToCppTranslation.h"
+
+namespace mlir {
+class DialectRegistry;
+
+/// Registers all dialects that can be translated to Cpp and the
+/// corresponding translation interfaces.
+static inline void registerAllToCppTranslations(DialectRegistry ®istry) {
+ registerBuiltinDialectCppTranslation(registry);
+ registerControlFlowDialectCppTranslation(registry);
+ registerEmitCDialectCppTranslation(registry);
+ registerFuncDialectCppTranslation(registry);
+}
+
+} // namespace mlir
+
+#endif // MLIR_TARGET_CPP_DIALECT_ALL_H
diff --git a/mlir/include/mlir/Target/Cpp/Dialect/Builtin/BuiltinToCppTranslation.h b/mlir/include/mlir/Target/Cpp/Dialect/Builtin/BuiltinToCppTranslation.h
new file mode 100644
index 00000000000000..25f19022f49043
--- /dev/null
+++ b/mlir/include/mlir/Target/Cpp/Dialect/Builtin/BuiltinToCppTranslation.h
@@ -0,0 +1,31 @@
+//===---- BuiltinToCppTranslation.h - Builtin Dialect to Cpp ----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This provides registration calls for Builtin dialect to Cpp translation.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TARGET_CPP_DIALECT_BUILTIN_BUILTINTOCPPTRANSLATION_H
+#define MLIR_TARGET_CPP_DIALECT_BUILTIN_BUILTINTOCPPTRANSLATION_H
+
+namespace mlir {
+
+class DialectRegistry;
+class MLIRContext;
+
+/// Register the Builtin dialect and the translation from it to the Cpp in the
+/// given registry;
+void registerBuiltinDialectCppTranslation(DialectRegistry ®istry);
+
+/// Register the Builtin dialect and the translation from it in the registry
+/// associated with the given context.
+void registerBuiltinDialectCppTranslation(MLIRContext &context);
+
+} // namespace mlir
+
+#endif // MLIR_TARGET_CPP_DIALECT_BUILTIN_BUILTINTOCPPTRANSLATION_H
diff --git a/mlir/include/mlir/Target/Cpp/Dialect/ControlFlow/ControlFlowToCppTranslation.h b/mlir/include/mlir/Target/Cpp/Dialect/ControlFlow/ControlFlowToCppTranslation.h
new file mode 100644
index 00000000000000..fea57fd1c02e28
--- /dev/null
+++ b/mlir/include/mlir/Target/Cpp/Dialect/ControlFlow/ControlFlowToCppTranslation.h
@@ -0,0 +1,31 @@
+//===- ControlFlowToCppTranslation.h - ControlFlow Dialect to Cpp -*-C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This provides registration calls for ControlFlow dialect to Cpp translation.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TARGET_CPP_DIALECT_CONTROLFLOW_CONTROLFLOWTOCPPTRANSLATION_H
+#define MLIR_TARGET_CPP_DIALECT_CONTROLFLOW_CONTROLFLOWTOCPPTRANSLATION_H
+
+namespace mlir {
+
+class DialectRegistry;
+class MLIRContext;
+
+/// Register the ControlFlow dialect and the translation from it to the Cpp in
+/// the given registry;
+void registerControlFlowDialectCppTranslation(DialectRegistry ®istry);
+
+/// Register the ControlFlow dialect and the translation from it in the registry
+/// associated with the given context.
+void registerControlFlowDialectCppTranslation(MLIRContext &context);
+
+} // namespace mlir
+
+#endif // MLIR_TARGET_CPP_DIALECT_CONTROLFLOW_CONTROLFLOWTOCPPTRANSLATION_H
diff --git a/mlir/include/mlir/Target/Cpp/Dialect/EmitC/EmitCToCppTranslation.h b/mlir/include/mlir/Target/Cpp/Dialect/EmitC/EmitCToCppTranslation.h
new file mode 100644
index 00000000000000..af47ffad1ab62e
--- /dev/null
+++ b/mlir/include/mlir/Target/Cpp/Dialect/EmitC/EmitCToCppTranslation.h
@@ -0,0 +1,31 @@
+//===------ EmitCToCppTranslation.h - EmitC Dialect to Cpp-------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This provides registration calls for EmitC dialect to Cpp translation.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TARGET_CPP_DIALECT_EMITC_EMITCTOCPPTRANSLATION_H
+#define MLIR_TARGET_CPP_DIALECT_EMITC_EMITCTOCPPTRANSLATION_H
+
+namespace mlir {
+
+class DialectRegistry;
+class MLIRContext;
+
+/// Register the EmitC dialect and the translation from it to the Cpp in the
+/// given registry;
+void registerEmitCDialectCppTranslation(DialectRegistry ®istry);
+
+/// Register the EmitC dialect and the translation from it in the registry
+/// associated with the given context.
+void registerEmitCDialectCppTranslation(MLIRContext &context);
+
+} // namespace mlir
+
+#endif // MLIR_TARGET_CPP_DIALECT_EMITC_EMITCTOCPPTRANSLATION_H
diff --git a/mlir/include/mlir/Target/Cpp/Dialect/Func/FuncToCppTranslation.h b/mlir/include/mlir/Target/Cpp/Dialect/Func/FuncToCppTranslation.h
new file mode 100644
index 00000000000000..b167093dc334bb
--- /dev/null
+++ b/mlir/include/mlir/Target/Cpp/Dialect/Func/FuncToCppTranslation.h
@@ -0,0 +1,31 @@
+//===------- FuncToCppTranslation.h - Func Dialect to Cpp -------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This provides registration calls for Func dialect to Cpp translation.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TARGET_CPP_DIALECT_FUNC_FUNCTOCPPTRANSLATION_H
+#define MLIR_TARGET_CPP_DIALECT_FUNC_FUNCTOCPPTRANSLATION_H
+
+namespace mlir {
+
+class DialectRegistry;
+class MLIRContext;
+
+/// Register the Func dialect and the translation from it to the Cpp in the
+/// given registry;
+void registerFuncDialectCppTranslation(DialectRegistry ®istry);
+
+/// Register the Func dialect and the translation from it in the registry
+/// associated with the given context.
+void registerFuncDialectCppTranslation(MLIRContext &context);
+
+} // namespace mlir
+
+#endif // MLIR_TARGET_CPP_DIALECT_FUNC_FUNCTOCPPTRANSLATION_H
diff --git a/mlir/include/mlir/Target/Cpp/TranslateToCpp.h b/mlir/include/mlir/Target/Cpp/TranslateToCpp.h
new file mode 100644
index 00000000000000..9deb44a9fe0066
--- /dev/null
+++ b/mlir/include/mlir/Target/Cpp/TranslateToCpp.h
@@ -0,0 +1,194 @@
+//===-- TranslateToCpp.h - Define CppEmitter struct -------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the CppEmitter meta struct used for Cpp translation.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TARGET_CPP_TRANSLATETOCPP_H
+#define MLIR_TARGET_CPP_TRANSLATETOCPP_H
+
+#include "mlir/Dialect/EmitC/IR/EmitC.h"
+#include "mlir/IR/BuiltinTypes.h"
+#include "mlir/Support/IndentedOstream.h"
+#include "mlir/Target/Cpp/CppEmitter.h"
+#include "mlir/Target/Cpp/CppTranslationInterface.h"
+#include "llvm/ADT/ScopedHashTable.h"
+#include "llvm/Support/raw_ostream.h"
+#include <stack>
+
+namespace mlir {
+
+/// Emitter that uses dialect specific emitters to emit C++ code.
+struct CppEmitter {
+ friend LogicalResult translateToCpp(Operation *op, raw_ostream &os,
+ bool declareVariablesAtTop);
+
+ explicit CppEmitter(raw_ostream &os, Operation *module,
+ bool declareVariablesAtTop);
+
+ /// Emits attribute or returns failure.
+ LogicalResult emitAttribute(Location loc, Attribute attr);
+
+ /// Emits operation 'op' with/without training semicolon or returns failure.
+ LogicalResult emitOperation(Operation &op, bool trailingSemicolon);
+
+ /// Emits type 'type' or returns failure.
+ LogicalResult emitType(Location loc, Type type);
+
+ /// Emits array of types as a std::tuple of the emitted types.
+ /// - emits void for an empty array;
+ /// - emits the type of the only element for arrays of size one;
+ /// - emits a std::tuple otherwise;
+ LogicalResult emitTypes(Location loc, ArrayRef<Type> types);
+
+ /// Emits array of types as a std::tuple of the emitted types independently of
+ /// the array size.
+ LogicalResult emitTupleType(Location loc, ArrayRef<Type> types);
+
+ /// Emits an assignment for a variable which has been declared previously.
+ LogicalResult emitVariableAssignment(OpResult result);
+
+ /// Emits a variable declaration for a result of an operation.
+ LogicalResult emitVariableDeclaration(OpResult result,
+ bool trailingSemicolon);
+
+ /// Emits a declaration of a variable with the given type and name.
+ LogicalResult emitVariableDeclaration(Location loc, Type type,
+ StringRef name);
+
+ /// Emits the variable declaration and assignment prefix for 'op'.
+ /// - emits separate variable followed by std::tie for multi-valued operation;
+ /// - emits single type followed by variable for single result;
+ /// - emits nothing if no value produced by op;
+ /// Emits final '=' operator where a type is produced. Returns failure if
+ /// any result type could not be converted.
+ LogicalResult emitAssignPrefix(Operation &op);
+
+ /// Emits a label for the block.
+ LogicalResult emitLabel(Block &block);
+
+ /// Emits the operands and atttributes of the operation. All operands are
+ /// emitted first and then all attributes in alphabetical order.
+ LogicalResult emitOperandsAndAttributes(Operation &op,
+ ArrayRef<StringRef> exclude = {});
+
+ /// Emits the operands of the operation. All operands are emitted in order.
+ LogicalResult emitOperands(Operation &op);
+
+ /// Emits value as an operands of an operation
+ LogicalResult emitOperand(Value value);
+
+ /// Emit an expression as a C expression.
+ LogicalResult emitExpression(emitc::ExpressionOp expressionOp);
+
+ /// Return the existing or a new name for a Value.
+ StringRef getOrCreateName(Value val);
+
+ /// Return the existing or a new label of a Block.
+ StringRef getOrCreateName(Block &block);
+
+ /// Whether to map an mlir integer to a unsigned integer in C++.
+ bool shouldMapToUnsigned(IntegerType::SignednessSemantics val);
+
+ /// RAII helper function to manage entering/exiting C++ scopes.
+ struct Scope {
+ Scope(CppEmitter &emitter)
+ : valueMapperScope(emitter.valueMapper),
+ blockMapperScope(emitter.blockMapper), emitter(emitter) {
+ emitter.valueInScopeCount.push(emitter.valueInScopeCount.top());
+ emitter.labelInScopeCount.push(emitter.labelInScopeCount.top());
+ }
+ ~Scope() {
+ emitter.valueInScopeCount.pop();
+ emitter.labelInScopeCount.pop();
+ }
+
+ private:
+ llvm::ScopedHashTableScope<Value, std::string> valueMapperScope;
+ llvm::ScopedHashTableScope<Block *, std::string> blockMapperScope;
+ CppEmitter &emitter;
+ };
+
+ /// Returns wether the Value is assigned to a C++ variable in the scope.
+ bool hasValueInScope(Value val);
+
+ // Returns whether a label is assigned to the block.
+ bool hasBlockLabel(Block &block);
+
+ /// Returns the output stream.
+ raw_indented_ostream &ostream() { return os; };
+
+ /// Returns if all variables for op results and basic block argu...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This refactors the MLIR TranslateToCpp part by introducing CppTranslationInterface that breaks down the overall translation into different dialects. This majorly helps the readability of the code and extensibility for future c++ translation addition.
weizchen
force-pushed
the
refactor-mlir-cpp-translation
branch
from
192078c to
3437b30
Compare
|
|
||
| namespace mlir { | ||
| namespace emitc { |
There was a problem hiding this comment.
Why losing the namespace here?
There was a problem hiding this comment.
oh I thought it might be good to let translateToCpp be shared with other dialects (not "owned" by emitc). But now a second thought appears that emitc is the end goal for the cpp translation so I could also add it back to keep it original.
| #include "mlir/IR/DialectInterface.h" | ||
| #include "mlir/Support/LogicalResult.h" | ||
|
|
||
| namespace mlir { |
There was a problem hiding this comment.
This is all part of EmitC still right? If so let's keep the namespace.
There was a problem hiding this comment.
yeah, using emitc namespace also makes sense to me
|
|
||
| /// Convenience functions to produce interleaved output with functions returning | ||
| /// a LogicalResult. This is different than those in STLExtras as functions used | ||
| /// on each element doesn't return a string. |
There was a problem hiding this comment.
I don't get this comment, the interleaveComma in STLExtras takes a functor that does not return a string either
There was a problem hiding this comment.
This part actually comes from the original EmitC code, I tried to keep most of the code as original as possible but I could double check.
| raw_ostream &os, | ||
| UnaryFunctor eachFn) { | ||
| return interleaveWithError(c.begin(), c.end(), eachFn, [&]() { os << ", "; }); | ||
| } |
There was a problem hiding this comment.
This does not belong here, it seems like a generic MLIR utility.
| #include "mlir/Dialect/EmitC/IR/EmitC.h" | ||
| #include "mlir/Target/Cpp/TranslateToCpp.h" | ||
|
|
||
| using namespace mlir; |
There was a problem hiding this comment.
That should be in any header.
|
As I understand the current design of emitc, it's intended that the translator will only accept the emitc dialect, and other dialects are instead supposed to convert into emitc before translation. Your PR wants to move away from that design, correct? |
Hi, thank you for your comment. I also thought about this before but I think not necessarily, I would not say this PR will conflict with the current design of emitc. Although in this PR, dialects like func getting separated out seems like we want to do direct translation to cpp, but as emitc func gets more functionality, we could still remove func and only use emitc. I think this will be more useful as people aim for a more customized cpp translation in the future. |
|
Thank you for your work @weizchen. It is undeniable that the emitter can be improved. However, after a first glance, I think there are both conceptual and technical issues. I'm afraid that this will definitely require some discussions and iterations before it could potentially be merged. Let me provide more context and some background information. When we proposed the emitter for upstream inclusion, the were concerns and push back mainly due to the API of the emitter. At that time, the consensus was to keep the surface of the emitter as small as possible. This also made it necessary for us to fork the emitter for our use case in IREE. However, we have now evolved the EmitC dialect to a point where we no longer need a forked emitter, as we can achieve what we need through conversions. Hence, @simon-camp recently removed the forked emitter from IREE with PR #16357@IREE. The intention is to remove all non-EmitC operations from the emitter. Support for operations of the SCF dialect were removed with PR #68206 and we recently accepted PR #83798 that removed support for the Arith dialect. With PR #79612, I added a func, call and return operation. The only reason for us not yet removing support for the Func dialects ops from emitter is to not break downstream users as EmitC's operations don't support multiple return values. Instead of extending the emitter, we aim to further develop the dialect in a way that everything can be accomplished through conversions. To me, it seems that the proposed changes do not align with this intention. Furthermore, there are some mechanical issues. For example the implementation in |
|
Hi @marbre, thank you so much for this detailed explanation and references! I totally agree that discussion is needed on this PR's implementation.
I also discover the trend of adding more conversions to EmitC to remove direct dialect translations which totally makes sense to me. I agree that separating out dialects like Func in this PR does not align with the current emitc strategy but what I vision for this is we have a fully supported emitter from EmitC Dialect, all main dialects are first translated to EmitC and then to C code. While the
And for this, I really struggled creating such Utils file, especially as the translations are all moving towards the EmitC Dialect. The original idea was to use this file for similar op translations to avoid code duplication (like arith constant and emitc constant), and I want to keep This PR actually originated from a need to translate our customized dialect to c++ where we found current emitc structure is a bit hard to inherit and reuse. So we think that with this, cpp translation might be more sustainable and extensible. I wonder what do you think of this, your idea is highly valued. Thanks! |
It would be interesting to understand why you can't use the dialect conversion mechanism to lower to EmitC actually: can you come up with examples? |
Hi, thank you for the response and your previous reviews! Regarding the question, one reason is the dialect is designed to fully target its C API, so going through EmitC adds one layer of overhead as things get a bit complicated to represent customized C operations, types and attributes with opaque families. I like the opaque and verbatim ops as they solve issues but I cannot really do much with them. For example like a compound type or like |
I suspect we'd want to understand this with much more details, with in-tree examples, before opening the door to bespoke emitters. As I mentioned before "the solution may also be to extend EmitC concepts", but I think we won't know about this if we just expose interfaces without first building a good understanding of the limits. |
Yeah sorry for not making this clear. So for example, the compound type I mentioned before, |
|
I think it would be really valuable if you could post an IR snippet @weizchen. It is of course possible, and even likely, that we're missing some ops in the EmitC dialect. It's developed and extended on a need-driven basis. However, as @joker-eph mentioned, it would be helpful to better understand this before exposing the interfaces. Maybe there is a convenient way using conversions that opens up if we add some more missing peaces to the dialect. |
Sure thing! Here is an IR snippet that has a similar idea with the Cache_Level example I talked about. This TQue type comprises one enum attribute: TPosition (VECIN, VECOUT...) that represents hardware positions and a buffer number. // void type1(TQue<QuePosition::VECIN, 1> que1) {
func.func @type1(%arg0: !ascendc.TQue<VECIN, 1>) {
return
}
// void type2(TQue<QuePosition::VECIN, 2> que1) {
func.func @type2(%arg0: !ascendc.TQue<VECIN, 2>) {
return
}
// void type3(TQue<QuePosition::VECIN, 3> que1) {
func.func @type3(%arg0: !ascendc.TQue<VECIN, 3>) {
return
}
// void type4(TQue<QuePosition::VECOUT, 2> que1) {
func.func @type4(%arg0: !ascendc.TQue<VECOUT, 2>) {
return
}
// void type5(TQue<QuePosition::A1, 2> que1) {
func.func @type5(%arg0: !ascendc.TQue<A1, 2>) {
return
}
// void type6(TQue<QuePosition::C1, 2> que1) {
func.func @type6(%arg0: !ascendc.TQue<C1, 2>) {
return
}
Please correct me if I am missing anything, but in my understanding, if we want to lower this TQue type to EmitC Dialect, we have to lower it to opaque type one by one. So for the six types above, I would need six different conversions to opaque, while this number can greatly increase with different combinations of TPosition and buffer number. But if we can support custom printing, for example like |
|
Couldn't you have something like this: std::string typeName = std::string("TQue<QuePosition::") + std::to_string(tque.getTposition()) + ", " + std::to_string(tque.getBufNum()) + ">";
Type t = emitc::OpaqueType::get(ctx, typeName);That's not much different to printing the parts to the output stream. |
Ah you are right, thank you for your answer! Another thing I found is the member functions. Suppose TQue has some member functions that we want to call. For example, in mlir, we have |
Unfortunately, there is no convenient way to express a member access operator. While this could be added to the dialect, we hesitated to do so, as this is closely related to an lvalue/rvalue discussion we're currently facing (@simon-camp will post an update to this later on discourse). Alongside we would need to add function pointers (related to TL;DR, right know there is no convenient way, but you could use |
Here is the mentioned discourse thread. |
|
I see, thank you all for the help. I will adopt the EmitC conversion to see how it goes. For the purpose of this PR, I think the extension could always be rediscussed if more need comes up for customization in the future if necessary. For now I will close this PR, thanks again for the info and the help! |
Thanks for the discussion! Feel free to open up topics at discourse for any further EmitC related discussions or requests. |
Hi, this aims to refactor the MLIR TranslateToCpp part by introducing CppTranslationInterface that breaks down the overall translation into separate dialect cpp translations. It follows the way that LLVMTranslation is constructed in MLIR.
I believe this improves the overall readability of code structure while could help with the extensibility for future c++ translation addition like using vector dialect to translate to ACLE or other vector extensions for X86 or RISC-V. Could also be further adjusted for supporting cpp translation with custom dialects.
I tried to keep most of the code to be as original as possible while here are some of the desisions I make:
CppEmitterto load the context for collecting relevant interfaces.printConstantOp()is only used for emitc now, but I think it looks clearer this way.