This is an archive of the discontinued LLVM Phabricator instance.

[mlir][complex] Convert complex.abs to libm
ClosedPublic

Authored by Lewuathe on Jun 10 2022, 12:25 AM.

Details

Summary

Convert complex.abs to libm library

Diff Detail

Event Timeline

Lewuathe created this revision.Jun 10 2022, 12:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2022, 12:25 AM
Lewuathe requested review of this revision.Jun 10 2022, 12:25 AM
bixia added inline comments.Jun 17 2022, 3:10 PM
mlir/lib/Conversion/ComplexToLibm/ComplexToLibm.cpp
69

This template class implementation is very similar to ScalarOpToLibmCall, with the only difference that one handle operations that produce a complex type as a result while the other handles operations that produce a float type as a result. It would be nice if we can somehow combine these two template classes into one.

@bixia Thank you for the feedback. I tried to combine them into one template function but the following error keeps showing up. Looks like complex::AbsOp fails to be embedded into the function in ScalarOpToLibmCall. I think we may need to change the op definition somewhat. Do you have any idea about that?

Assertion failed: (isa<U>()), function cast, file Types.h, line 251.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /Users/sasakikai/dev/llvm-project/build/bin/mlir-opt /Users/sasakikai/dev/llvm-project/mlir/test/Conversion/ComplexToLibm/convert-to-libm.mlir -convert-complex-to-libm -canonicalize
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  mlir-opt                 0x000000010bd95a8a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 42
1  mlir-opt                 0x000000010bd94928 llvm::sys::RunSignalHandlers() + 248
2  mlir-opt                 0x000000010bd96120 SignalHandler(int) + 288
3  libsystem_platform.dylib 0x00007ff80e9d2dfd _sigtramp + 29
4  libsystem_platform.dylib 0x00007f9225042b20 _sigtramp + 18446743635998735680
5  libsystem_c.dylib        0x00007ff80e908d24 abort + 123
6  libsystem_c.dylib        0x00007ff80e9080cb err + 0
7  mlir-opt                 0x000000010d2fc003 (anonymous namespace)::ScalarOpToLibmCall<mlir::complex::AbsOp>::matchAndRewrite(mlir::complex::AbsOp, mlir::PatternRewriter&) const (.cold.10) + 35
8  mlir-opt                 0x000000010c8c3d29 (anonymous namespace)::ScalarOpToLibmCall<mlir::complex::AbsOp>::matchAndRewrite(mlir::complex::AbsOp, mlir::PatternRewriter&) const + 1257
9  mlir-opt                 0x000000010cf667d0 mlir::PatternApplicator::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&, llvm::function_ref<bool (mlir::Pattern const&)>, llvm::function_ref<void (mlir::Pattern const&)>, llvm::function_ref<mlir::LogicalResult (mlir::Pattern const&)>) + 1552
10 mlir-opt                 0x000000010cd51e92 (anonymous namespace)::OperationLegalizer::legalize(mlir::Operation*, mlir::ConversionPatternRewriter&) + 2290
11 mlir-opt                 0x000000010cd4a43d (anonymous namespace)::OperationConverter::convertOperations(llvm::ArrayRef<mlir::Operation*>, llvm::function_ref<void (mlir::Diagnostic&)>) + 1117
12 mlir-opt                 0x000000010cd4ce9b mlir::applyPartialConversion(mlir::Operation*, mlir::ConversionTarget&, mlir::FrozenRewritePatternSet const&, llvm::DenseSet<mlir::Operation*, llvm::DenseMapInfo<mlir::Operation*, void>>*) + 75
13 mlir-opt                 0x000000010c8c410d (anonymous namespace)::ConvertComplexToLibmPass::runOnOperation() + 669
14 mlir-opt                 0x000000010cd0da95 mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) + 485
15 mlir-opt                 0x000000010cd0e147 mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) + 295
16 mlir-opt                 0x000000010cd0fb72 mlir::PassManager::run(mlir::Operation*) + 642
17 mlir-opt                 0x000000010cce4377 performActions(llvm::raw_ostream&, bool, bool, llvm::SourceMgr&, mlir::MLIRContext*, llvm::function_ref<mlir::LogicalResult (mlir::PassManager&)>) + 503
18 mlir-opt                 0x000000010cce20c1 processBuffer(llvm::raw_ostream&, std::__1::unique_ptr<llvm::MemoryBuffer, std::__1::default_delete<llvm::MemoryBuffer>>, bool, bool, bool, bool, llvm::function_ref<mlir::LogicalResult (mlir::PassManager&)>, mlir::DialectRegistry&, llvm::ThreadPool*) + 641
19 mlir-opt                 0x000000010cce1d66 mlir::MlirOptMain(llvm::raw_ostream&, std::__1::unique_ptr<llvm::MemoryBuffer, std::__1::default_delete<llvm::MemoryBuffer>>, llvm::function_ref<mlir::LogicalResult (mlir::PassManager&)>, mlir::DialectRegistry&, bool, bool, bool, bool, bool) + 182
20 mlir-opt                 0x000000010cce2bcd mlir::MlirOptMain(int, char**, llvm::StringRef, mlir::DialectRegistry&, bool) + 2525
21 mlir-opt                 0x000000010bc2a9e6 main + 198
22 dyld                     0x0000000111ccf51e start + 462
bixia added a comment.Jun 27 2022, 8:55 AM

How did you combine the two template? I was thinking the current template only take one parameter Op. The new template will take one additional parameter, that is, a functor that returns the return-type from Op.

The result type is used for defining the function type.

auto opFunctionTy = FunctionType::get(
    rewriter.getContext(), op->getOperandTypes(), op->getResultTypes());

I thought abs op returns the float-like type so that we do not need to pass the functor to the template explicitly. Is this assumption wrong?

Lewuathe updated this revision to Diff 440850.Jun 28 2022, 10:19 PM

Reuse single template function for complex.abs lowering.

Lewuathe updated this revision to Diff 440851.Jun 28 2022, 10:21 PM

Remove unnecessary type declaration.

Lewuathe updated this revision to Diff 440855.Jun 28 2022, 10:26 PM

Rename functors.

bixia added inline comments.Jun 29 2022, 2:23 PM
mlir/lib/Conversion/ComplexToLibm/ComplexToLibm.cpp
46–48

Would you please update the document to describe the new parameter, such as it must correspond to the result type of the operation?
You may also want to rename the parameter and assign a default value to it, such as TypeResolver = ComplexTypeResolver.

65

Would this help make it clear that the struct is used for the result type of the op?
s/given complex type/given complex result type/

67–68

This can be simplified to return a boolean, such as true for float and false for double.
If you make this change, you may also want to rename the struct, such as ComplexTypeResolver, FloatTypeResolver.

78

Similar to the above.

Lewuathe added inline comments.Jun 29 2022, 11:38 PM
mlir/lib/Conversion/ComplexToLibm/ComplexToLibm.cpp
67–68

We should return some value specifying the error for the incompatible element type. If we change the signature to bool, we can only return true or false which does not keep error information.

Lewuathe updated this revision to Diff 441276.Jun 29 2022, 11:39 PM

Enhance docs to clarify the usage of FuncResolver.

bixia added inline comments.Jul 1 2022, 1:11 PM
mlir/lib/Conversion/ComplexToLibm/ComplexToLibm.cpp
67–68

can you use std::optional<bool> for the return type

Good idea! Thanks let me try.

Lewuathe updated this revision to Diff 441996.Jul 3 2022, 7:14 PM

Use llvm::Optional since MLIR looks mostly using llvm::Optional.

@Lewuathe, thank you for all your contributions to MLIR and CompexOps dialect! Would you be interested also in adding a dialect-specific attribute for complex numbers so that complex.constant would accept a dedicated attribute, e.g. F32ComplexAttr instead of ArrayAttr. That would make the code cleaner and also enable more opportunities for folding.

bixia accepted this revision.Jul 7 2022, 3:11 PM
This revision is now accepted and ready to land.Jul 7 2022, 3:11 PM

@pifon2a Sounds interesting. Let me try to work on that. Do you have any specific idea on the interface of the custom attribute for complex types? (e.g. complex.constant<1.0, 2.0> ?)

This revision was automatically updated to reflect the committed changes.

@pifon2a Sounds interesting. Let me try to work on that. Do you have any specific idea on the interface of the custom attribute for complex types? (e.g. complex.constant<1.0, 2.0> ?)

Thank you! I think if we can print/parse #complex.number<1.0, 2.0>: f32, then it would be pretty readable and tidy. Implementation-wise, this attribute should be pretty close to Builtin_FloatAttr, but with 3 parameters instead of 2.

@pifon2a Got it. Thanks for sharing your idea.