This is an archive of the discontinued LLVM Phabricator instance.

[flang] Use mlir complex dialect for supported operations
ClosedPublic

Authored by DavidTruby on Oct 13 2022, 8:24 AM.

Details

Summary

This patch lowers the complex operations supported by the MLIR complex
dialect to those operations rather than libm. When the math runtime flag
is set to precise, libm lowering is used instead.

Diff Detail

Event Timeline

DavidTruby created this revision.Oct 13 2022, 8:24 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 13 2022, 8:24 AM
DavidTruby requested review of this revision.Oct 13 2022, 8:24 AM

Remove unnecessary convert.

DavidTruby added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3512

I think this variable is mis-spelt. I didn't change it here because I didn't think it was in scope for this patch. I guess this should be fixed as a separate commit? Or I can just tack it on to this one.

vzakhari added inline comments.Oct 17 2022, 12:37 PM
flang/lib/Lower/IntrinsicCall.cpp
1325

Can you please put this whole change under an engineering option? You may enable it by default, but I would like to have a way to switch to the old behavior easily to deal with any issues in our testing. I may take an AR to remove the option, when eveything works well.

1327

It is better to just return result here and avoid the else clause.

1336
1348

Does it mean a Complex dialect operation will return fir::ComplexType result? Shouldn't it be mlir::ComplexType?

This works for abs, but it does not look correct in general.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
3512

My bad. I am okay with fixing it here.

3514

Alternatively, you may call populateComplexToStandardConversionPatterns around line 3395, since ComplexToStandard convertor does not modify parent operations.

DavidTruby added inline comments.Oct 17 2022, 6:16 PM
flang/lib/Lower/IntrinsicCall.cpp
1325

Happy to do this, the only reason I didn't is that I thought it was already covered by setting the fp-contract to precise. Did you want something more fine-grained for just this option?

1348

I think you're right, we should take this result as the mlir::ComplexType and convert it to a fir::ComplexType. I'll update with this change.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
3514

Originally I did this, but it interacts with MathToFuncsPass and so needs to be run in the same pipeline as that. I couldn't see a better way of doing that than putting it here. When I did put it around 3395 as populateComplexToLLVMConversionPatterns it didn't work for this reason.

vzakhari added inline comments.Oct 17 2022, 6:25 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3514

Hmm, it should not interact with MathToFuncs, because MathToFuncs currently only work for math::IPowIOp. I wonder in which way it did not work. Is it possible that due to the ordering we ended up converting some complex ops into LLVM rather than to Standard?

vzakhari added inline comments.Oct 20 2022, 12:00 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3514

Just want to clarify that my previous comment did not intend to hold back the change. I have no remarks besides line 1342.

Add flag to control new behaviour.
Take results of operations as mlir::complex and convert to fir::complex.

clang-format

Thank you for the changes! LGTM with minor remarks.

flang/include/flang/Optimizer/Support/InitFIR.h
26

Why use '<...>'? Typo?

flang/lib/Lower/IntrinsicCall.cpp
43

Same here - why '<...>'?

1340

Still recommend specifying some fixed allocation size for SmallVector, e.g. 2.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
40

'<...>' again.

Fix includes
Add size suggestion to SmallVector

flang/include/flang/Optimizer/Support/InitFIR.h
26

Yep, my mistake

flang/lib/Lower/IntrinsicCall.cpp
1340

I actually thought I'd made this change but it must have slipped past my commit!

vzakhari accepted this revision.Oct 25 2022, 10:53 AM

Thanks! LGTM

This revision is now accepted and ready to land.Oct 25 2022, 10:53 AM

Thanks! LGTM

Where does D134364 stand after this? Do we still need that design document?
Thanks

This broke my build:

ld.lld: error: undefined symbol: mlir::createConvertComplexToStandardPass()
>>> referenced by CodeGen.cpp:3540 (/llvm-project/flang/lib/Optimizer/CodeGen/CodeGen.cpp:3540)
>>>               CMakeFiles/obj.FIRCodeGen.dir/CodeGen.cpp.o:((anonymous namespace)::FIRToLLVMLowering::runOnOperation())

ld.lld: error: undefined symbol: mlir::populateComplexToLLVMConversionPatterns(mlir::LLVMTypeConverter&, mlir::RewritePatternSet&)
>>> referenced by CodeGen.cpp:3605 (/llvm-project/flang/lib/Optimizer/CodeGen/CodeGen.cpp:3605)
>>>               CMakeFiles/obj.FIRCodeGen.dir/CodeGen.cpp.o:((anonymous namespace)::FIRToLLVMLowering::runOnOperation())
collect2: error: ld returned 1 exit status

There are missing dependencies in CMake files. David, I can fix it, if you do not have time to do this today.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
3514

This requires linking MLIRComplexToStandard in CMakeLists.txt.

3579

This requires linking MLIRComplexToLLVM in CMakeLists.txt.

Thanks! LGTM

Where does D134364 stand after this? Do we still need that design document?
Thanks

I think we should still merge the document. We need to update it mentioning special handling for some operations (like power).

This broke my build:

ld.lld: error: undefined symbol: mlir::createConvertComplexToStandardPass()
>>> referenced by CodeGen.cpp:3540 (/llvm-project/flang/lib/Optimizer/CodeGen/CodeGen.cpp:3540)
>>>               CMakeFiles/obj.FIRCodeGen.dir/CodeGen.cpp.o:((anonymous namespace)::FIRToLLVMLowering::runOnOperation())

ld.lld: error: undefined symbol: mlir::populateComplexToLLVMConversionPatterns(mlir::LLVMTypeConverter&, mlir::RewritePatternSet&)
>>> referenced by CodeGen.cpp:3605 (/llvm-project/flang/lib/Optimizer/CodeGen/CodeGen.cpp:3605)
>>>               CMakeFiles/obj.FIRCodeGen.dir/CodeGen.cpp.o:((anonymous namespace)::FIRToLLVMLowering::runOnOperation())
collect2: error: ld returned 1 exit status

There are missing dependencies in CMake files. David, I can fix it, if you do not have time to do this today.

I created https://reviews.llvm.org/D136773 to fix this.