Page MenuHomePhabricator

[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
1564

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.

1347

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
1548

Please document the operands.

1957

Please document the operands.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1905 ↗(On Diff #261148)

dyn_cast -> cast

bondhugula added inline comments.Apr 30 2020, 2:36 AM
mlir/test/mlir-cpu-runner/complex_numbers.mlir
6 ↗(On Diff #261148)

CHECK-LABEL to prevent things from matching past this.

21 ↗(On Diff #261148)

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 ↗(On Diff #261148)

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 ↗(On Diff #261148)

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
1008

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

1011

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

1553

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

1561

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

1963

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.

1332

We tend to use the kRealIdxInCpxNumStruct style for constants.

1345

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 ↗(On Diff #261183)

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
1 ↗(On Diff #261183)

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
1011

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
1011

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
991

Same comment as below.

1011

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

1541

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

1951

Same here.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1228 ↗(On Diff #261183)

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

1649 ↗(On Diff #261183)

Same comment as the other verifier.

1905 ↗(On Diff #261183)

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
1008

Also changed $imag to $imaginary.

1011

Nice. Thanks!

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

Thanks!

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

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

mlir/test/IR/invalid-ops.mlir
1236

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.