Page MenuHomePhabricator

Add support for complex constants to MLIR core.
ClosedPublic

Authored by akuegel on May 5 2021, 7:27 AM.

Diff Detail

Event Timeline

akuegel created this revision.May 5 2021, 7:27 AM
akuegel requested review of this revision.May 5 2021, 7:27 AM

I probably need to add more tests, and maybe need to improve some things. Still, I wanted to already send this out to get feedback on the general approach here.
The RFC with more discussion is here:
https://llvm.discourse.group/t/rfc-add-a-builtin-complexattr/3316/28

rriddle added inline comments.May 5 2021, 10:39 AM
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1966

nit: Drop trivial braces here.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
1071
1085
mlir/lib/IR/BuiltinAttributes.cpp
589

You shouldn't need vector here, can you not use ArrayRef directly, which supports creation from an initializer list? You might need to explicitly type that values as Attribute.

598

Same here.

mehdi_amini added inline comments.May 5 2021, 2:00 PM
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1968

I think you also need to check more here, in particular that the two are the same type to form a valid complex?

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

Are the changes around standard constant exercised in a test?
Otherwise can you add this, including for the verifier errors added below.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
113–114

If you don't intend to support anything else than a complex, more invariants checking is necessary.
But also we should likely have a verifier on the dialect itself.

119

Can you replace auto with the types here? (except the first one)

akuegel updated this revision to Diff 344364.May 11 2021, 5:15 AM

Address review comments. Start adding more tests.

akuegel marked 6 inline comments as done.May 11 2021, 5:24 AM

Thanks for the review comments :) And sorry for taking so long to respond. I was busy with some other tasks.

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

I had looked around but didn't find tests for valid constants. I have now added tests for complex constants to Standard/ops.mlir. There were already tests for invalid constants in Standard/invalid.mlir, and I added additional tests to cover the new logic.

mlir/lib/IR/BuiltinAttributes.cpp
589

Thanks for the suggestion :) Done.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
113–114

So if I understand correctly, most of the checks should be done in the Verifier. Is there another one except LLVM::ConstantOp verifier that I would have to adapt?
I just added an additional check here that the array attribute has size 2. I think the other things are checked via the recursive call.

mehdi_amini added inline comments.May 11 2021, 10:03 AM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
113–114

Yes you're right, the verifier will be enough.

However the verifier checks an MLIR type:
if (auto complexTy = op.getType().dyn_cast<ComplexType>()) { while here you only have a "struct" as input. So it isn't clear to me that it is totally redundant?

115

What is this catching/exercising by the way?
Why isn't it an error?

akuegel updated this revision to Diff 344782.May 12 2021, 5:32 AM

Improve verifier logic, add tests.

akuegel added inline comments.May 12 2021, 5:41 AM
mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
335

I have added this to be able to return an error for nested structs.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
113–114

Good point, I didn't realize that I need to check for LLVMStructType in the verifier. I think I understand a bit better now how the interaction of the verifier and ModuleTranslation is. I hope the code I added makes now more sense :)

115

Yes, that should be an error. But this logic is now also part of the verifier.
I have added some logic though for checking whether the created constants that the struct will consist of have the correct type. I can possibly also check this in the verifier instead, but it might be a bit harder to do there.

akuegel updated this revision to Diff 345391.May 14 2021, 4:01 AM

Fix assertion error triggered in new test.

It seems one of the tests I have added triggered an assertion error. I added a check that the FloatAttr has the correct type before calling ConstantFP::get.

Please take another look :)

akuegel updated this revision to Diff 345401.May 14 2021, 5:06 AM

Fix ClangTidy warning.

mehdi_amini accepted this revision.May 14 2021, 1:01 PM

LG, thanks!

This revision is now accepted and ready to land.May 14 2021, 1:01 PM
This revision was automatically updated to reflect the committed changes.