This is an archive of the discontinued LLVM Phabricator instance.

[mlir][DenseElementsAttr] Add support for ComplexType elements
ClosedPublic

Authored by rriddle on May 1 2020, 7:48 PM.

Details

Summary

This revision adds support for storing ComplexType elements inside of a DenseElementsAttr. We store complex objects as an array of two elements, matching the definition of std::complex. There is no current attribute storage for ComplexType, but DenseElementsAttr provides API for access/creation using std::complex<>. Given that the internal implementation of DenseElementsAttr is already fairly opaque, the only real complexity here is in the printing/parsing. This revision keeps it simple for now and always uses hex when printing complex elements. A followup will add prettier syntax for this.

Diff Detail

Event Timeline

rriddle created this revision.May 1 2020, 7:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2020, 7:48 PM
jpienaar added inline comments.May 4 2020, 9:01 AM
mlir/include/mlir/IR/Attributes.h
1057

Comments need update?

1133

Same

mlir/lib/IR/AttributeDetail.h
378

This is a little bit weird. I see some use C64 to refer to 2x64 bit float complex number while XLA consider std::complex<float> as C64 (so 64 is total). So seems the bitwidth is overloaded for complex already, as long as you document it this seems fine then as there doesn't seem to be 1 convention.

rriddle marked 4 inline comments as done.May 4 2020, 10:33 AM
rriddle added inline comments.
mlir/lib/IR/AttributeDetail.h
378

That is different. We don't suffer from the problematic naming because we explicitly specify the element type. We specify the type exactly the same as C++ complex, i.e. there is no C64 there is only complex<double>/complex<float>/etc. It doesn't seem like there is any ambiguity there?

rriddle updated this revision to Diff 261867.May 4 2020, 10:33 AM
rriddle marked an inline comment as done.

Resolve comments

jpienaar accepted this revision.May 5 2020, 9:41 AM
jpienaar marked 2 inline comments as done.

Looks good thanks

mlir/lib/IR/AttributeDetail.h
378

You are returning the bitwidth of the storage as element bitwidth which could either be the bitwidth of how it is stored or or the elements. But you are correct in that if one looks at the comment in 375, this would seem to return the storage bitwidth rather than anything intrinsic about the type (e.g., we are storing bf16 as 64 bits :-)). So no ambiguity.

mlir/lib/Parser/Parser.cpp
2068

Now the i vs j debate could heat up :)

This revision is now accepted and ready to land.May 5 2020, 9:41 AM
This revision was automatically updated to reflect the committed changes.