This is an archive of the discontinued LLVM Phabricator instance.

[mlir][complex] Initial support for FastMath flag when converting to LLVM
ClosedPublic

Authored by Lewuathe on Jul 26 2023, 4:12 AM.

Details

Summary

This change contains the initial support of FastMath flag in complex dialect. Similar to what we did in Arith dialect, fastmath attributes in the complex dialect are directly mapped to the corresponding LLVM fastmath flags.

In this diff,

  • Definition of FastMathAttr as a custom attribute in the Complex dialect that inherits from the EnumAttr class.
  • Definition of ComplexFastMathInterface, which is an interface that is implemented by operations that have a complex::fastmath attribute.
  • Declaration of a default-valued fastmath attribute for unary and arithmetic operations in the Complex dialect.
  • Conversion code to lower arithmetic fastmath flags to LLVM fastmath flags

NOT in this diff (but planned and progressively implemented):

  • Documentation of flag meanings
  • Support the fastmath flag conversion to Arith dialect
  • Folding/rewrite implementations that are enabled by fastmath flags (although it's the original motivation to support the flag)

RFC: https://discourse.llvm.org/t/rfc-fastmath-flags-support-in-complex-dialect/71981

Diff Detail

Event Timeline

Lewuathe created this revision.Jul 26 2023, 4:12 AM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Lewuathe requested review of this revision.Jul 26 2023, 4:12 AM
Lewuathe edited the summary of this revision. (Show Details)Jul 26 2023, 4:17 AM
Lewuathe edited the summary of this revision. (Show Details)
Matt added a subscriber: Matt.Jul 26 2023, 10:34 AM
kiranchandramohan added inline comments.
mlir/include/mlir/Dialect/Complex/IR/ComplexBase.td
29–55 ↗(On Diff #544295)

Isn't it possible to reuse these definitions from the Arithmetic Dialect (ArithBase.td)?

+r Rob for complex

mehdi_amini added inline comments.Jul 27 2023, 8:07 PM
mlir/include/mlir/Dialect/Complex/IR/ComplexBase.td
29–55 ↗(On Diff #544295)

+1: I'd like to not see a new attribute here, see https://discourse.llvm.org/t/place-for-fastmath-attributes/69933

Thanks. I'll change to reuse the flag definition in Arith dialect.

Lewuathe updated this revision to Diff 547109.Aug 3 2023, 11:51 PM

Reuse the fastmath flag definitions in arith dialect.

Lewuathe updated this revision to Diff 547113.Aug 4 2023, 12:07 AM

Remove unused header files.

@kiranchandramohan @kuhar Could you give me your eyes on this change when you get a chance?

I think we can reuse ArithFastMathInterface and ComplexFastMathInterface is not required. The functions in ComplexFastMathInterface seem to be the same as in ArithFastMathInterface. Was there any specific reason to define ComplexFastMathInterface? If not, I think reusing ArithFastMathInterface is the way forward.

mlir/include/mlir/Dialect/Complex/IR/CMakeLists.txt
9–12 ↗(On Diff #547113)

I think the ComplexOpsInterface is not required.

mlir/include/mlir/Dialect/Complex/IR/Complex.h
28–33

This can be removed.

mlir/include/mlir/Dialect/Complex/IR/ComplexOps.td
15

This can be replaced with ArithOpsInterfaces.td.

27–29
mlir/include/mlir/Dialect/Complex/IR/ComplexOpsInterfaces.td
1 ↗(On Diff #547113)

This Interface is not needed, I think. We can reuse ArithFastMathInterface.

mlir/lib/Conversion/ComplexToLLVM/ComplexToLLVM.cpp
62

This function seems to be available from mlir/Conversion/ArithCommon/AttrToLLVMConverter.h. Could you reuse that function?

You might have to add MLIRArithAttrToLLVMConversion if you go this route.

Lewuathe updated this revision to Diff 550202.Aug 15 2023, 12:00 AM

Reuse interface and conversion function.

Lewuathe updated this revision to Diff 550206.Aug 15 2023, 12:18 AM

Remove unued headers.

Lewuathe marked 4 inline comments as done.Aug 15 2023, 12:18 AM

Please update the test flang/test/Lower/complex-operations.f90 to fix the CI. Some minor comments inline.

LG. Please wait a couple of days before you submit to give others a chance to review.

mlir/lib/Conversion/ComplexToLLVM/ComplexToLLVM.cpp
189–192

Nit: Expand this type.

220–223

Nit: Expand this type.

268–271

Nit: Expand this type.

308–311

Nit: Expand this type.

mlir/lib/Dialect/Complex/IR/CMakeLists.txt
18

Nit: This was in alphabetical order.

mlir/test/Conversion/ComplexToLLVM/convert-to-llvm.mlir
263

Is a test for complex.abs missing?

This revision is now accepted and ready to land.Aug 15 2023, 2:15 AM
Lewuathe updated this revision to Diff 550530.Aug 15 2023, 4:31 PM

Be aware of the default Flang fastmath flag. https://reviews.llvm.org/D138675

Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2023, 4:31 PM
Lewuathe updated this revision to Diff 550563.Aug 15 2023, 5:57 PM

Post review follow-up

Lewuathe updated this revision to Diff 550564.Aug 15 2023, 5:58 PM
Lewuathe marked 6 inline comments as done.

Remove unused code.

Lewuathe updated this revision to Diff 550590.Aug 15 2023, 8:57 PM

Specify the fastmath flag explicitly in flang tests.

@kiranchandramohan Thank you so much for the review! I'll keep this change for a few days accordingly.