This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Start simple EmitC dialect.
AbandonedPublic

Authored by jpienaar on Mar 22 2020, 11:34 AM.

Details

Summary

Add EmitC dialect that has trivial mapping to C++. This dialect does not attempt
to model C++ semantics but just syntax to make it easy to translate out (e.g., it
attempts to make emitting simple textual code simpler). EmitC is used to add
simple C++ target that can currently only emit functions and function calls as
C++ code. This is useful for cases where you have a C++ compiler but no
access to the codegen (e.g., for using MLIR optimizations along with legacy or
proprietary systems) and for prototyping/debugging (e.g., found this useful to
play with shape functions). The emitter mostly defers to EmitC ops with the
exception of supporting FuncOp, some standard attributes and lowers
multi-result functions/ops are lowered using std::tuple.

Only test to verify emit happens, not that emitted code can be compiled, will
follow up with an integration test.

Previous discussions:
https://groups.google.com/a/tensorflow.org/d/msg/mlir/iOAB10Us6uo/wJ_pVQ_qBwAJ
https://llvm.discourse.group/t/simple-c-emitter-rev/773/4

Diff Detail

Event Timeline

jpienaar created this revision.Mar 22 2020, 11:34 AM
rriddle requested changes to this revision.Mar 22 2020, 12:31 PM
rriddle added inline comments.
mlir/include/mlir/Target/Cpp.h
94

StringRef

104

nit: Why not just inline this type? It has one use.

116

I don't think this friend declaration is necessary.

125

nit: Can we get comments on these variables?

131

nit: Can you add documentation to this class?

156

The description mentions multiple emitters per dialect, but here you are keying on the dialect namespace.

mlir/lib/Target/Cpp/TranslateToCpp.cpp
11

I think several of these can be trimmed, as they are already included transitively.

32

op.emitError() for all of these?

45

This seems like debug output.

59

nit: Remove trivial braces.

62

nit: Just return *mapper.begin()

70

nit: return emitters.lookup(dialect);

89

nit: Can you hoist the functor out of the call for readability?

97

nit: llvm::is_contained()

100

This seems incorrect to have inside of the loop.

130

Remove the extraneous semi-colon.

168

I don't understand why this is a failure case, can you add comments?

212

Do we really need to expose this? Seems like this provided by an overload of 'emit' that doesn't have an 'emitters' argument.

mlir/lib/Target/Cpp/TranslateToCppRegistration.cpp
11

Some of these includes are unnecessary.

25

Please wrap these in anonymous namespaces.

35

These methods should be marked static.

This revision now requires changes to proceed.Mar 22 2020, 12:31 PM
jpienaar updated this revision to Diff 251941.Mar 22 2020, 9:24 PM
jpienaar marked 24 inline comments as done.

Addressed review comments

jpienaar marked an inline comment as done.Mar 22 2020, 9:27 PM

Updated, thanks

mlir/include/mlir/Target/Cpp.h
116

It is required for valueInScopeCount access (which honestly is more to enable making it simple to reuse variable numbers).

156

Updated comment above register function.

mlir/lib/Target/Cpp/TranslateToCpp.cpp
168

Just changed to propagate to fallback behavior.

212

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.

rriddle added inline comments.Mar 22 2020, 11:35 PM
mlir/include/mlir/Target/Cpp.h
116

What I mean is, the elements of a class already visible to nested classes(i.e. implicitly friend):
https://stackoverflow.com/questions/14758881/access-of-nested-classes-which-behave-like-friends-but-arent/14759027#14759027

jpienaar updated this revision to Diff 252057.Mar 23 2020, 8:28 AM

Removing unneeded friend declaration

jpienaar marked an inline comment as done.Mar 23 2020, 8:29 AM
stephenneuendorffer added inline comments.
mlir/include/mlir/Target/Cpp.h
40

What do you think about introducing an "EmitC" dialect, rather than doing string catenation here, isolate it somewhere else?

mlir/lib/Target/Cpp/TranslateToCpp.cpp
198–208

Maybe an Interface would work well here, instead of having a separate registry? I've used this to build a simulator for some dialects and it seems to scale reasonably well.

Can you discuss this on Discourse?

jpienaar marked 2 inline comments as done.Mar 23 2020, 2:23 PM

Can you discuss this on Discourse?

Sure

mlir/include/mlir/Target/Cpp.h
40

That would be very nice, but would be much larger effort, you'd need to be able to handle lambdas, templates and many different things that a dialect would want to emit.

mlir/lib/Target/Cpp/TranslateToCpp.cpp
198–208

That is discussed in the rev description, making it an interface is a bit more invasive and requires one lowering for a given op, while one could have multiple completely different emissions for the same op, which is why I went for a separate registry.

jpienaar updated this revision to Diff 252345.Mar 24 2020, 9:09 AM

Try example with simple dialect with 1:1 mapping to translation output. It doesn't model the language, only emitting to it.

marbre added a subscriber: marbre.Mar 25 2020, 6:30 AM
jpienaar retitled this revision from [mlir] Start simple C++ backend to [mlir] Start simple EmitC dialect..Mar 26 2020, 10:06 AM
jpienaar edited the summary of this revision. (Show Details)
mehdi_amini added inline comments.Mar 27 2020, 1:23 PM
mlir/test/Target/cpp-calls.mlir
17

I am a bit confused by the design of this dialect. I don't really get why we need a dialect for expressing this instead of... just emitting the C++ code directly for example.

What other ops would go into the "emitc" dialect?

jpienaar marked an inline comment as done.Mar 27 2020, 3:35 PM
jpienaar added inline comments.
mlir/test/Target/cpp-calls.mlir
17

This would correspond mostly to the syntax, to make the emission of the ops independent of the syntax. I think it is open to me how much value this dialect adds compared to jsut emitting + helper functions (previous version). So this is more an example, if folks don't see value then we can go back to the previous version.

pyprogrammer added inline comments.
mlir/lib/Target/Cpp/TranslateToCpp.cpp
355–366

Seems like a reasonable location to use mlir::TypeSwitch instead of having a long chain of if / typechecks.

vnvo added a subscriber: vnvo.Apr 22 2021, 7:18 AM
Herald added a project: Restricted Project. · View Herald Transcript
jpienaar abandoned this revision.Sep 10 2021, 10:36 AM

Follow up work has superseded this :)