-
Notifications
You must be signed in to change notification settings - Fork 22
#1851: Add options to ttmlir-translate
to output C++ from TTKernel dialect
#1602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d77935c
to
3a50482
Compare
ecfef6c
to
fbd3a0d
Compare
emitKernelAsCpp(mlir::ModuleOp op, llvm::raw_ostream &os, const ttkernel::ThreadType &threadType ) | ||
{ | ||
std::vector<func::FuncOp> ops; | ||
op->walk([&](func::FuncOp entry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to extract the nested FuncOp
from ModuleOp
?
//TODO: Should generalize this to read kernel type from Attribute? | ||
void registerTensixKernelToCpp() { | ||
TranslateFromMLIRRegistration reg( | ||
"tensixkernel-to-cpp", "translate tensix kernel to C++", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel like I need to change the option to ttkernel-to-cpp=noc
and ttkernel-to-cpp=tensix
Is this possible? If so, where can I find an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only know how to do this using the -opt
tool, the translate MLIR tool seems much less featureful in terms of CLI. Maybe we could do --ttkernel-to-cpp-tensix
and --ttkernel-to-cpp-noc
? It's probably OK the way you have it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am more for what @nsmithtt suggested, it aligns better with the rest of the codebase:
--ttkernel-to-cpp-tensix
--ttkernel-to-cpp-noc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ttmlir-translate
ttmlir-translate
ttmlir-translate
to output C++ from TTKernel dialect
ttmlir-translate
to output C++ from TTKernel dialectttmlir-translate
to output C++ from TTKernel dialect
22391b4
to
b608099
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Bill, sorry I was on vacation, still catching up on reviews. This looks great a couple of minor nits and comments inline, but all optional.
|
||
namespace mlir::tt::ttkernel { | ||
|
||
enum class ThreadType : uint32_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should we include "ttmlir/Dialect/TTKernel/IR/TTKernelOpsTypes.h"
instead of doing a forward declare? Would also prevent having an enum passed by reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK let's do it since compile times aren't a concern here most likely
//TODO: Should generalize this to read kernel type from Attribute? | ||
void registerTensixKernelToCpp() { | ||
TranslateFromMLIRRegistration reg( | ||
"tensixkernel-to-cpp", "translate tensix kernel to C++", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only know how to do this using the -opt
tool, the translate MLIR tool seems much less featureful in terms of CLI. Maybe we could do --ttkernel-to-cpp-tensix
and --ttkernel-to-cpp-noc
? It's probably OK the way you have it too.
|
||
namespace mlir::tt::ttkernel { | ||
|
||
// TODO: Should generalize this to read kernel type from Attribute? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true too, we could attach an attribute to the func op via op->setDiscardableAttr
.
I think the IR would like:
func.func @ttkernel_noc() -> () attributes { ttkernel.thread_type = #ttkernel.thread_type<tensix> } {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments that I would like your follow up.
//TODO: Should generalize this to read kernel type from Attribute? | ||
void registerTensixKernelToCpp() { | ||
TranslateFromMLIRRegistration reg( | ||
"tensixkernel-to-cpp", "translate tensix kernel to C++", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am more for what @nsmithtt suggested, it aligns better with the rest of the codebase:
--ttkernel-to-cpp-tensix
--ttkernel-to-cpp-noc
64553fd
to
d3c6898
Compare
|
||
// Translates a TTKernel operation to C++ and writes it to the given | ||
// stream. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
#include "mlir/Dialect/EmitC/IR/EmitC.h" | ||
#include "mlir/Dialect/Func/IR/FuncOps.h" | ||
#include "mlir/Tools/mlir-translate/Translation.h" | ||
|
||
#include "mlir/Dialect/SCF/IR/SCF.h" | ||
#include "ttmlir/Dialect/TT/IR/TT.h" | ||
#include "ttmlir/Dialect/TTKernel/IR/TTKernel.h" | ||
#include "ttmlir/Dialect/TTKernel/IR/TTKernelOpsTypes.h" | ||
#include "ttmlir/Target/TTKernel/TTKernelToCpp.h" | ||
#include <mlir/Dialect/MemRef/IR/MemRef.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok done
d3c6898
to
1e0a8c6
Compare
ttmlir-translate
to output C++ from TTKernel dialectttmlir-translate
to output C++ from TTKernel dialect
Shardon requires emitting C++ from python kernels. To help with the testing and debug process, I've added a TTKernel to C++ target and options to
ttmlir-translate
to directly dump C++ from TTKernel dialect.There are separate Noc and Tensix kernel dumping options because shardon users need to explicitly control the definition of Noc and Tensix kernels separately.