-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[mlir][emitC] Add support to emitter for classop, fieldop and getfieldop
#145605
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
classop, 'fieldop and getfieldop` correctlyclassop, fieldop and getfieldop correctly
|
@llvm/pr-subscribers-mlir-emitc @llvm/pr-subscribers-mlir Author: Jaden Angella (Jaddyen) ChangesExtends the emitter to accurately emit classes from classops, fieldops and getfieldops Full diff: https://github.com/llvm/llvm-project/pull/145605.diff 2 Files Affected:
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index 067a0470b14e4..9cd30eae8d36d 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -997,6 +997,61 @@ static LogicalResult printOperation(CppEmitter &emitter, ModuleOp moduleOp) {
return success();
}
+static LogicalResult printOperation(CppEmitter &emitter, ClassOp classOp) {
+ CppEmitter::Scope classScope(emitter);
+ raw_indented_ostream &os = emitter.ostream();
+ os << "class " << classOp.getSymName() << " final {\n";
+ os << "public:\n\n";
+
+ os.indent();
+ os << "const std::map<std::string, char*> _buffer_map {\n";
+ for (Operation &op : classOp) {
+ if (auto fieldOp = dyn_cast<FieldOp>(op))
+ os << " { \"" << fieldOp.getSymName() << "\", reinterpret_cast<char*>(&"
+ << fieldOp.getAttrs() << ") },\n";
+ }
+ os << "};\n";
+
+ os << "char* getBufferForName(const std::string& name) const {\n";
+ os << " auto it = _buffer_map.find(name);\n";
+ os << " return (it == _buffer_map.end()) ? nullptr : it->second;\n";
+ os << "}\n\n";
+ for (Operation &op : classOp) {
+ if (failed(emitter.emitOperation(op, /*trailingSemicolon=*/false)))
+ return failure();
+ }
+
+ os.unindent();
+ os << "};";
+ return success();
+}
+
+static LogicalResult printOperation(CppEmitter &emitter, FieldOp fieldOp) {
+ raw_ostream &os = emitter.ostream();
+ if (failed(emitter.emitType(fieldOp->getLoc(), fieldOp.getType())))
+ return failure();
+ os << " " << fieldOp.getSymName() << ";";
+ return success();
+}
+
+static LogicalResult printOperation(CppEmitter &emitter,
+ GetFieldOp getFieldOp) {
+ raw_indented_ostream &os = emitter.ostream();
+ Location loc = getFieldOp->getLoc();
+
+ if (getFieldOp->getNumResults() > 0) {
+ Value result = getFieldOp->getResult(0);
+ if (failed(emitter.emitType(loc, result.getType())))
+ return failure();
+ os << " ";
+ if (failed(emitter.emitOperand(result)))
+ return failure();
+ os << " = ";
+ }
+ os << getFieldOp.getFieldName().str();
+ return success();
+}
+
static LogicalResult printOperation(CppEmitter &emitter, FileOp file) {
if (!emitter.shouldEmitFile(file))
return success();
@@ -1612,7 +1667,8 @@ LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) {
emitc::LoadOp, emitc::LogicalAndOp, emitc::LogicalNotOp,
emitc::LogicalOrOp, emitc::MulOp, emitc::RemOp, emitc::ReturnOp,
emitc::SubOp, emitc::SwitchOp, emitc::UnaryMinusOp,
- emitc::UnaryPlusOp, emitc::VariableOp, emitc::VerbatimOp>(
+ emitc::UnaryPlusOp, emitc::VariableOp, emitc::VerbatimOp,
+ emitc::ClassOp, emitc::FieldOp, emitc::GetFieldOp>(
[&](auto op) { return printOperation(*this, op); })
// Func ops.
diff --git a/mlir/test/mlir-translate/emitc_classops.mlir b/mlir/test/mlir-translate/emitc_classops.mlir
new file mode 100644
index 0000000000000..3232954f1149a
--- /dev/null
+++ b/mlir/test/mlir-translate/emitc_classops.mlir
@@ -0,0 +1,36 @@
+// RUN: mlir-translate --mlir-to-cpp %s | FileCheck %s
+
+emitc.class @modelClass {
+ emitc.field @input_tensor : !emitc.array<1xf32>
+ emitc.field @some_feature : !emitc.array<1xf32> {emitc.opaque = ["some_feature"]}
+ emitc.func @execute() {
+ %0 = "emitc.constant"() <{value = 0 : index}> : () -> !emitc.size_t
+ %1 = get_field @input_tensor : !emitc.array<1xf32>
+ %2 = get_field @some_feature : !emitc.array<1xf32>
+ %3 = subscript %1[%0] : (!emitc.array<1xf32>, !emitc.size_t) -> !emitc.lvalue<f32>
+ return
+ }
+}
+
+// CHECK: class modelClass final {
+// CHECK-NEXT: public:
+// CHECK-EMPTY:
+// CHECK-NEXT: const std::map<std::string, char*> _buffer_map {
+// CHECK-NEXT: { "input_tensor", reinterpret_cast<char*>(&None) },
+// CHECK-NEXT: { "some_feature", reinterpret_cast<char*>(&{emitc.opaque = ["some_feature"]}) },
+// CHECK-NEXT: };
+// CHECK-NEXT: char* getBufferForName(const std::string& name) const {
+// CHECK-NEXT: auto it = _buffer_map.find(name);
+// CHECK-NEXT: return (it == _buffer_map.end()) ? nullptr : it->second;
+// CHECK-NEXT: }
+// CHECK-EMPTY:
+// CHECK-NEXT: float[1] input_tensor;
+// CHECK-NEXT: float[1] some_feature;
+// CHECK-NEXT: void execute() {
+// CHECK-NEXT: size_t v1 = 0;
+// CHECK-NEXT: float[1] v2 = input_tensor;
+// CHECK-NEXT: float[1] v3 = some_feature;
+// CHECK-NEXT: return;
+// CHECK-NEXT: }
+// CHECK-EMPTY:
+// CHECK-NEXT: };
|
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.
The reflection bits should be predicated on there being those attributes.
classop, fieldop and getfieldop correctlyclassop, fieldop and getfieldop
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.
add a second test where the fields don't have the attribute - everything should work, except no reflection support.
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.
emitc.name_hint not emitc.opaque, no?
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.
yes!
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 does not seem specific to ClassOp but some usage of it. So these should be introduced before translation.
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.
ack, will do in different step.
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.
Lets keep this list sorted.
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.
Why final?
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 think this may be specific to the MLGO use-case. Perhaps that could be something that could be encoded in the ClassOp? Then its just a matter of lowering/refining the IR correctly ahead of time, like for other constructs.
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.
yes, we can specify in the ClassOp so this print allows for more use cases.
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.
Yes, this could either be an attribute of type ::mlir::UnitAttr or implemented similar to the specifiers attribute (type ::mlir::ArrayAttr) of the emitc::FuncOp.
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.
These don't look correct.
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.
thanks for pointing this out! will handle this when working on buffer_map op in next stage
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 think this may be specific to the MLGO use-case. Perhaps that could be something that could be encoded in the ClassOp? Then its just a matter of lowering/refining the IR correctly ahead of time, like for other constructs.
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.
Should these lines be indented manually? It's certainly easier to read, but you could end up w/ inconsistent indentation, right?
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.
true! it would be nicer to indent using the os. since i am deferring the emission of the buffermap to a stage where we have a buffermapop, i will handle this at that point.
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.
Isn't GetFieldOp defined to have 1 result? (
| let results = (outs AnyTypeOf<[EmitC_ArrayType, EmitC_LValueType]>:$result); |
Can also do here getFieldOp.getResult() rather than using the generic accessor on Operation.
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.
yes it is!
thanks for the pointer.
ive addressed this now.
Add support to the emitter for
ClassOp,FieldOpandGetFieldOp.These ops were introduced in #141158