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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

Looks good in general! I have some advice on the op syntax, feel free to argue against it.



================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1013
+  let arguments = (ins AnyFloat:$real, AnyFloat:$imag);
+  let results = (outs Complex<AnyFloat>:$cplx);
+
----------------
Could we use a full word for the name, `$complex`. We generate functions with this name, it's more readable but reasonably short


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1016
+  let assemblyFormat = [{
+    $real `,` $imag attr-dict `:` type($real) `,` type($imag) `->` type($cplx)
+  }];
----------------
Do we want to support "mixed" types, e.g. f32, f64 -> complex<f64>?  If not, I would just keep the `type($cplx)` here.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1528
+    ```mlir
+    %a = im %b, %c : complex<f32> -> f32
+    ```
----------------
This example looks wrong: the spec below says there is only one operand


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1536
+  let assemblyFormat = [{
+    $cplx attr-dict `:` type($cplx) `->` type($imag)
+  }];
----------------
I think `complex<X> -> X` is quite redundant here, keeping just the complex type should suffice.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1939
+    ```mlir
+    %a = re %b, %c : complex<f32> -> f32
+    ```
----------------
Same as above


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:197
+  FloatType elementType = type.getElementType().cast<FloatType>();
+  LLVM::LLVMType llvmElementType =
+      convertFloatType(elementType).cast<LLVM::LLVMType>();
----------------
I think you can just call convertType(type.getElementType()) and let it be dispatched for you.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1299
+
+const int REAL_IDX_IN_CPLX_NUM_STRUCT = 0;
+const int IMAG_IDX_IN_CPLX_NUM_STRUCT = 1;
----------------
We tend to use the `kRealIdxInCpxNumStruct` style for constants.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1312
+
+    // Pack real and imaginary part into one structure.
+    auto loc = op->getLoc();
----------------
Could you rather derive a new `ComplexStructBuilder` from `StructBuilder and use it here?  Look at MemRefDescriptor for an example.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:1230
+    return op.emitOpError(
+        "expected two operand types and complex element type to be the same");
+  return success();
----------------
Could you please add a test for this?  test/IR/invalid-ops.mlir sounds like the right file (although the file itself is misplaced)


================
Comment at: mlir/test/Conversion/StandardToLLVM/convert-to-llvmir.mlir:78
+func @complex_numbers() {
+^bb0:
+  %real0 = constant 1.2 : f32
----------------
You don't need an explicit label for the entry block in MLIR functions (and most other operations)


================
Comment at: mlir/test/mlir-cpu-runner/complex_numbers.mlir:1
+// RUN: mlir-opt %s -convert-std-to-llvm \
+// RUN: | mlir-cpu-runner -e create_and_extract_real -entry-point-result=f32 \
----------------
I think the conversion is straightforward enough to trust the individual components and drop this test. The `-runner` tests are intended to check that the runner itself works, your patch does not alter it in any way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79159