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
3354

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
1324

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.

1326

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

1335
1347

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
3354

My bad. I am okay with fixing it here.

3356

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
1324

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?

1347

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
3356

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
3356

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
3356

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
24

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

flang/lib/Lower/IntrinsicCall.cpp
42

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

1339

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
24

Yep, my mistake

flang/lib/Lower/IntrinsicCall.cpp
1339

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
3356

This requires linking MLIRComplexToStandard in CMakeLists.txt.

3395

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.