This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add EmitC dialect
ClosedPublic

Authored by marbre on Jun 9 2021, 8:52 AM.

Details

Summary

This upstreams the EmitC dialect and the corresponding Cpp target, both
initially presented with [1], from [2] to MLIR core. For the related
discussion, see [3].

[1] https://reviews.llvm.org/D76571
[2] https://github.com/iml130/mlir-emitc
[3] https://llvm.discourse.group/t/emitc-generating-c-c-from-mlir/3388

Co-authored-by: Jacques Pienaar <jpienaar@google.com>
Co-authored-by: Simon Camphausen <simon.camphausen@iml.fraunhofer.de>
Co-authored-by: Oliver Scherf <oliver.scherf@iml.fraunhofer.de>

Diff Detail

Event Timeline

marbre created this revision.Jun 9 2021, 8:52 AM
marbre requested review of this revision.Jun 9 2021, 8:52 AM
rriddle requested changes to this revision.Jun 10 2021, 6:24 PM

Thanks, starting to work through it.

Can you split this commit into multiple smaller commits? I think the translation and dialect parts can be separated.

mlir/include/mlir/Dialect/EmitC/IR/EmitC.h
17

What is used from here?

20

This header isn't necessary.

mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
34

Generally the Attributes/Dialect/Ops/Types are in separate .td files.

37

A very common idiom is to pass this as a template arg to the base class.

55–71

Can you move these to the .cpp file?

99–115

Same here and below.

139

Can you move this to the EmitC_Op class?

163

Why not constant? That generally matches the naming in the rest of the ecosystem.

166–169

Why do you reference std.constant here? Is that necessary? (std.constant may eventually, hopefully, go away)

181

nit: include or Include?

mlir/include/mlir/Target/Cpp/Cpp.h
1 ↗(On Diff #350913)

Cpp seems a bit too general given that this is seemingly only focused on EmitC.

25 ↗(On Diff #350913)

Can you document this?

122 ↗(On Diff #350913)

Can you use an active phrasing? shouldMapToSigned?

185 ↗(On Diff #350913)

Please fix the naming on these.

mlir/lib/Dialect/EmitC/IR/EmitC.cpp
53

nit: Drop all of the mlir::, you also don't need the emitc for quite a few of these either.

57

nit: Drop the trivial braces here.

74

Drop trivial braces here and throughought.

rriddle added inline comments.Jun 10 2021, 6:24 PM
mlir/lib/Dialect/EmitC/IR/EmitC.cpp
80–98

Can you do something to reduce the nesting here? In general, we prefer "early exit" style coding.

137

nit: Drop this line, it doesn't add anything.

mlir/lib/Target/Cpp/TranslateRegistration.cpp
19 ↗(On Diff #350913)
28 ↗(On Diff #350913)

nit: Drop the llvm:: here and below.

mlir/lib/Target/Cpp/TranslateToCpp.cpp
29 ↗(On Diff #350913)

Only the very first part of this function needs to know what ConstOpTy is, can you refactor to avoid a template?

32 ↗(On Diff #350913)
This revision now requires changes to proceed.Jun 10 2021, 6:24 PM
marbre updated this revision to Diff 351867.Jun 14 2021, 7:33 AM

Address review comments

Removes the target. This will be included in a separate commit.

jpienaar added inline comments.Jun 14 2021, 7:45 AM
mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
35

This is for applying a prefix unary operator? What is the benefit of this one op vs an AddressOf and DereferenceOp ? (And yeah with operator overloading these may not do that ... StarPrefix or the like is probably more generally correct)

67

Given opaque types, should there be a restriction added in use of this op? If one were to create a constant of a C++ class with internal state, then it would have side effects I think.

mlir/lib/Dialect/EmitC/IR/EmitC.cpp
1

Only need to specify file type in the .h files

marbre marked 15 inline comments as done.Jun 14 2021, 7:57 AM

Thanks for the review @rriddle. I hope this addresses most suggestions.

I will sent a separate patch that contains the target (your suggestions already applied), but like for additional comments with view on the folloiwing:
I agree that Cpp/Cpp.h isn't an appropriate name, which we simply adopted from @jpienaar's original patch. Do you have a suggestion? Should the name correspond to the dialect which is origin of the translation? In that case one should probably also rename the Cpp directory.
Regarding the use of namespace qualifiers in the target registration, in order to drop the namespace mlir {, we would either need to include mlir/InitAllTranslations.h or we forward declare in the cpp file itself:

namespace mlir {
void registerToCppTranslation()
void registerToCTranslation()
}

Is that what you propose? Right now the registration closely resembles target registrations like registerToSPIRVTranslation().

mlir/include/mlir/Dialect/EmitC/IR/EmitC.h
17

The IncludeOp as HasParent<"ModuleOp"> set.

20

Dropped it.

mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
34

Split those into EmitC, EmitCBase, EmitCAttrDefs and EmitCTypes.

37

Thanks for the hint, done.

163

Why not constant? That generally matches the naming in the rest of the ecosystem.

I wasn't aware that std.constant is possibly going to vanish. Rephrased accordingly.

166–169

Why do you reference std.constant here? Is that necessary? (std.constant may eventually, hopefully, go away)

I renamed to emitc.constant as I have no preference. I think I implemented this op after I looked into the TOSA where the op is indeed called const.

mlir/lib/Dialect/EmitC/IR/EmitC.cpp
53

Thanks, (hopefully) all done.

80–98

I have refactored to at least drop one level of nesting.

marbre marked 8 inline comments as done.Jun 14 2021, 8:03 AM
marbre added inline comments.Jun 14 2021, 8:16 AM
mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
35

We limited this to the use of the unary operators * and &. I have no strong preference and we had a GetAddressOfOp, but to me the feedback in an after the ODM indicated that an ApplyOp would be the better fit.

67

I am open to suggestions :) We can drop the NoSideEffect of course.

Thanks, took another pass. Probably just one more after this one.

mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
16

nit: I'd probably go with EmitCAttributes to match the EmitCTypes filename.

17

This include isn't necessary

29
40
58

Should this use a DenseElementsAttr instead? (Dense storage is generally more efficient)

60
71

Remaining reference to std.constant here?

72–73

The operation is marked with AnyAttr and AnyType though, is it supposed to be constrained to just emitc types? Or is this just remarking that the emitc types are supported in addition to everything else?

mlir/include/mlir/Dialect/EmitC/IR/EmitCAttrDefs.td
45

Can you move this to the .cpp? So that is next to the parser code.

mlir/include/mlir/Dialect/EmitC/IR/EmitCTypes.td
44

Can you move this to the .cpp? So that is next to the parser code.

mlir/lib/Dialect/EmitC/IR/EmitC.cpp
10

What is used from StandardOps?

15

I think some of these includes are unneccesary.

60
75

Can you spell out auto here?

75–76
77

Same here.

92

Can you spell out auto here?

93–94

Attribute is a pointer-like class, prefer passing by value.

96

If you aren't using the result, use isa instead.

117

Can you spell out auto here?

149

Can you spell out auto here?

190

nit: Spell out auto here.

marbre marked 24 inline comments as done.Jun 15 2021, 7:54 AM
marbre added inline comments.
mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
16

Renamed to EmitCAttributes with the next revision.

17

Thanks, dropped it.

58

You can specific the order of operands by passing integer values of index types. In addition, you can specify attributes that get lowered to constants, e.g. floats. With different types occurring, I think we cannot use a DenseElementsAttr. Anyway, I think the description was misleading. Rephrased a little, hoping to pronounce the "and" different.

67

I dropped NoSideEffect with the next revision.

71

Thanks for catching this!

72–73

I intended to express the latter. Rephrased to make this clearer.

mlir/include/mlir/Dialect/EmitC/IR/EmitCAttrDefs.td
45

Sure, addressed with the next revision.

mlir/lib/Dialect/EmitC/IR/EmitC.cpp
1

Thanks, I have removed the string in this and also in all *.cpp files relevant for follow up patches.

10

Thanks for taking a careful look! Seems to be a leftover, removed it.

15

Thanks for catching this. Cleaned it up.

75

Spelled out here and in all other places occurring in EmitC.cpp.

96

Thanks for the suggestions. All done.

marbre updated this revision to Diff 352139.Jun 15 2021, 7:58 AM
marbre marked 11 inline comments as done.

Address review comments

  • Renames EmitCAttrDefs.td to EmitCAttributes.td
  • Strip unused includes
  • Moves printer from tblgen to cpp
  • Revise ConstantOp Drops NoSideEffect trait and rephrases the op description
  • Improves code style E.g. replaces auto and improves the CallOp verifier
rriddle accepted this revision.Jun 17 2021, 2:11 AM

Thanks a lot for the work! This looks fine to land and build on.

mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
72–73

I don't know if it is important to explicitly call out (given that they are all in the same dialect, you could probably convey the same by showing an example using those types. No strong preference either way, but as mentioned in another comment; Can you add some examples to the descriptions?

88–89

Can you add an example IR snippet to the descriptions for these operations?

93
mlir/include/mlir/Dialect/EmitC/IR/EmitCAttributes.td
22 ↗(On Diff #352139)
mlir/include/mlir/Dialect/EmitC/IR/EmitCTypes.td
24–25

.td files should also be wrapped at 80 characters.

mlir/lib/Dialect/EmitC/IR/EmitC.cpp
73

Drop the const here, attributes are (essentially) immutable by design.

80–82
89

Drop the const here, attributes are (essentially) immutable by design.

90

This loop doesn't guard against other types of attributes (such as external dialect attributes). Is this just guarding against some known unsupported types? Would it be better to use an allow list as opposed to an explicit deny list?

mlir/test/Dialect/EmitC/ops.mlir
2

Do you need the -allow-unregistered-dialect on this?

mlir/test/Dialect/EmitC/types.mlir
2

Do you need the -allow-unregistered-dialect on these?

This revision is now accepted and ready to land.Jun 17 2021, 2:11 AM

Thanks a lot for the work! This looks fine to land and build on.

Thanks a lot for the review! I addressed your latest comments within the last revision which I will update prior to landing the commit.

marbre marked 10 inline comments as done.Jun 18 2021, 6:28 AM
marbre added inline comments.
mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
72–73

Dropped the last sentence and added examples for all EmitC ops.

88–89

Sure :) Done for all ops.

mlir/include/mlir/Dialect/EmitC/IR/EmitCTypes.td
24–25

Fixed several lines where line breaks were necessary. Thanks for pointing out.

mlir/lib/Dialect/EmitC/IR/EmitC.cpp
90

Good catch! It does not. Refactored accordingly and now check for supported types.
With C++20 it is even legal to emit floating-point type template parameters.

mlir/test/Dialect/EmitC/ops.mlir
2

We did because of !custom<"int32_t"> and "bar", but I refactored and dropped -allow-unregistered-dialect`.

mlir/test/Dialect/EmitC/types.mlir
2

Dropped it.

marbre updated this revision to Diff 352994.Jun 18 2021, 6:29 AM
marbre marked 6 inline comments as done.

Address review comments before landing

This revision was automatically updated to reflect the committed changes.