[PATCH] D79159: [MLIR] Add complex numbers to standard dialect

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:18
 
+#include "mlir/IR/StandardTypes.h"
 #include "mlir/Transforms/DialectConversion.h"
----------------
Please use forward declarations instead of includes in header files when you can.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:996
+  let description = [{
+    Syntax:
+
----------------
Same comment as below.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1016
+  let assemblyFormat = [{
+    $real `,` $imag attr-dict `:` type($real) `,` type($imag) `->` type($cplx)
+  }];
----------------
ftynse wrote:
> frgossen wrote:
> > ftynse wrote:
> > > Do we want to support "mixed" types, e.g. f32, f64 -> complex<f64>?  If not, I would just keep the `type($cplx)` here.
> > I would love to have only `type($cplx)` here. 
> > The reason for the current format is that `tablegen` complained when I dropped the other types. 
> > I can fix this with a hand-written parser and printer but before I do that I want to ask if this is also possible with the `assemblyFormat` thingy. 
> > What is the elegant way to do this? 
> Indeed, auto-generated printer and parser needs a way to resolve _all_ types and cannot do so in this case. Sometimes, it can rely on the traits like `SameOperandAndResultTypes`, but not here. Ideally, we would have some way of specifying "type deduction rules"... Until that exists, you will need printer and parser functions. To make life easier, you can just take those generated by mlir-tblgen today and amend them a bit to deduce the types.
You can do that with `TypesMatchWith`, it was intended for this purpose..


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1516
+  let description = [{
+    Syntax:
+
----------------
Don't provide a syntax if you have the declarative assembly(assemblyFormat). It is included automatically in the docs.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1927
+  let description = [{
+    Syntax:
+
----------------
Same here.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:1228
+  auto elementType = op.getType().cast<ComplexType>().getElementType();
+  if (op.real().getType() != elementType || op.imag().getType() != elementType)
+    return op.emitOpError(
----------------
You should be able to verify this in tablegen with TypesMatchWith.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:1649
+      op.cplx().getType().cast<ComplexType>().getElementType();
+  if (complexElementType != op.imag().getType())
+    return op.emitOpError(
----------------
Same comment as the other verifier.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:1905
+  auto complexElementType =
+      op.cplx().getType().cast<ComplexType>().getElementType();
+  if (complexElementType != op.real().getType())
----------------
Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79159