This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add complex numbers to standard dialect
ClosedPublic

Authored by frgossen on Apr 30 2020, 1:52 AM.

Details

Summary

Add CreateComplexOp, ReOp, and ImOp to the standard dialect.
This is the first step to support complex numbers.

Diff Detail

Event Timeline

frgossen created this revision.Apr 30 2020, 1:52 AM
Herald added a project: Restricted Project. · View Herald Transcript

awesome! thanks!

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1541

verifier is already included in Std_Op, you don't have to add it here.

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
197

const can be omitted in LLVM codebase.

1314

auto loc = op->getLoc(); and reuse it 3 times?

bondhugula added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
161

These comments should all be triple /// I believe.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1525

Please document the operands.

1937

Please document the operands.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1905

dyn_cast -> cast

bondhugula added inline comments.Apr 30 2020, 2:36 AM
mlir/test/mlir-cpu-runner/complex_numbers.mlir
6

CHECK-LABEL to prevent things from matching past this.

21

CHECK-LABEL here please.

frgossen updated this revision to Diff 261183.Apr 30 2020, 3:57 AM
frgossen marked 9 inline comments as done.

Document operands, small changes.

frgossen added inline comments.Apr 30 2020, 3:59 AM
mlir/test/mlir-cpu-runner/complex_numbers.mlir
6

I double-checked the output of mlir-opt %s -convert-std-to-llvm | mlir-cpu-runner -e create_and_extract_real -entry-point-result=f32 and it is no more than the result of the computation, i.e. 1.200000e+00.
Because the two functions are compiled and executed separately, I believe matching past things is not an issue here and I don't see how I can use CHECK-LABEL in a useful way.
Do let me know if there is a more elegant way to write this test.

21

See above.

ftynse requested changes to this revision.Apr 30 2020, 4:13 AM

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

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1013

Could we use a full word for the name, $complex. We generate functions with this name, it's more readable but reasonably short

1016

Do we want to support "mixed" types, e.g. f32, f64 -> complex<f64>? If not, I would just keep the type($cplx) here.

1530

This example looks wrong: the spec below says there is only one operand

1538

I think complex<X> -> X is quite redundant here, keeping just the complex type should suffice.

1943

Same as above

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
197

I think you can just call convertType(type.getElementType()) and let it be dispatched for you.

1299

We tend to use the kRealIdxInCpxNumStruct style for constants.

1312

Could you rather derive a new ComplexStructBuilder from `StructBuilder and use it here? Look at MemRefDescriptor for an example.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1230

Could you please add a test for this? test/IR/invalid-ops.mlir sounds like the right file (although the file itself is misplaced)

mlir/test/Conversion/StandardToLLVM/convert-to-llvmir.mlir
78

You don't need an explicit label for the entry block in MLIR functions (and most other operations)

mlir/test/mlir-cpu-runner/complex_numbers.mlir
2

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.

This revision now requires changes to proceed.Apr 30 2020, 4:13 AM
frgossen marked an inline comment as done.Apr 30 2020, 4:27 AM

Thanks for the many helpful comments.
There is one comment for which I'd like clarification.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1016

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?

ftynse added inline comments.Apr 30 2020, 5:32 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1016

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.

rriddle added inline comments.Apr 30 2020, 10:37 AM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
18

Please use forward declarations instead of includes in header files when you can.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
996

Same comment as below.

1016

You can do that with TypesMatchWith, it was intended for this purpose..

1518

Don't provide a syntax if you have the declarative assembly(assemblyFormat). It is included automatically in the docs.

1931

Same here.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1228

You should be able to verify this in tablegen with TypesMatchWith.

1649

Same comment as the other verifier.

1905

Same here.

frgossen updated this revision to Diff 261766.May 4 2020, 2:52 AM
frgossen marked 24 inline comments as done.

Simplified assembly format, introduced ComplexStructBuilder.

frgossen added inline comments.May 4 2020, 3:02 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1013

Also changed $imag to $imaginary.

1016

Nice. Thanks!

ftynse accepted this revision.May 4 2020, 4:07 AM

Thanks!

mlir/include/mlir/Dialect/StandardOps/EDSC/Intrinsics.h
34

Nit: let's use std_re to be consistent with the assembly spelling.

mlir/test/IR/invalid-ops.mlir
1236 ↗(On Diff #261766)

Nit: let's not hardcode SSA value names in checks, I think it's fine to shorten the expected-error message to expects different type than prior uses: 'f32' vs 'f64'

This revision is now accepted and ready to land.May 4 2020, 4:07 AM
frgossen updated this revision to Diff 261778.May 4 2020, 4:22 AM
frgossen marked 2 inline comments as done.

Nits

Address nits.

frgossen updated this revision to Diff 261788.May 4 2020, 5:19 AM

Apply clang tidy suggestions

This revision was automatically updated to reflect the committed changes.