[PATCH] D79281: [mlir][DenseElementsAttr] Add support for ComplexType elements

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar accepted this revision.
jpienaar marked 2 inline comments as done.
jpienaar added a comment.
This revision is now accepted and ready to land.

Looks good thanks



================
Comment at: mlir/lib/IR/AttributeDetail.h:378
+  if (ComplexType complex = eltType.dyn_cast<ComplexType>())
+    return getDenseElementBitWidth(complex.getElementType()) * 2;
   // FIXME(b/121118307): using 64 bits for BF16 because it is currently stored
----------------
rriddle wrote:
> jpienaar wrote:
> > 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.
> 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?
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.


================
Comment at: mlir/lib/Parser/Parser.cpp:2068
+  // If parsing a complex type.
+  // TODO: Support complex elements with pretty element printing.
+  if (eltType.isa<ComplexType>()) {
----------------
Now the i vs j debate could heat up :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79281/new/

https://reviews.llvm.org/D79281