Page MenuHomePhabricator

[flang] Lower Fortran math intrinsic operations into MLIR ops or libm calls.
ClosedPublic

Authored by vzakhari on Jun 22 2022, 2:57 PM.

Details

Summary

Added new -lower-math-early option that defaults to 'true' that matches
the current math lowering scheme. If set to 'false', the intrinsic math
operations will be lowered to MLIR operations, which should potentially
enable more MLIR optimizations, or libm calls, if there is no corresponding
MLIR operation exists or if "precise" mode is requested.
The generated math MLIR operations are then converted to LLVM dialect
during codegen phase.

The -lower-math-early option is not exposed to users currently. I plan to
get rid of the "early" lowering completely, when "late" lowering
is robust enough to support all math intrinsics that are currently
supported via pgmath. So "late" mode will become default and -lower-math-early
option will not be needed. This will effectively eliminate the mandatory
dependency on pgmath in Fortran lowering, but this is WIP.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Please review.

I can extract Math dialect changes into a separate commit, if needed.

+1. Yes, that would be a fast way to get that portion of the patch in.

This is really great to see. I like using the MLIR math dialect operations and it also provides a pathway to not depend on external libraries.

Also, is this a one-off patch or do you plan to work further on this? Would we have to extend the math dialect to handle all the fortran math intrinsics? Have you explored using the complex dialect operations directly, like e.g. https://mlir.llvm.org/docs/Dialects/ComplexOps/#complexsin-mlircomplexsinop?

jeanPerier accepted this revision.Jun 23 2022, 2:44 AM

Looks great to me thanks !

If you do not split the patch, please get someone from MLIR core to team to also approve the Math dialect change.

This revision is now accepted and ready to land.Jun 23 2022, 2:44 AM

Hi @vzakhari !

Thanks for working on this. Before diving into details, I have a couple of design questions.

Question 1.
What's the end goal here? I can see how this could allow us to drop the dependency on libpgmath. Is that what you are aiming here for? What's the intended workflow for end users? We've discussed this before, see #1377. It would be good to clarify what points from that discussion are being addressed here.

Question 2.

Added new -math-lowering option that defaults to 'early' that matches the current math lowering scheme.

Could you clarify what you mean by this? There are 4 drivers in Flang (bbc, tco, flang-new and flang-new -fc1). From what I can see, you've used the llvm::cl API, but flang-new and flang-new -fc1 options are defined in Clang's Options.td. I've noticed that you've used -mmlir to forward these options, but -mmlir is intended for developers rather than end-users. So what would be the path for enabling this for end users? If this effort is to remove the dependency on libpgmath, then I think that it would make sense to make this available sooner rather than later.

Question 3.
It's good to see that you are testing with both flang-new and bbc, but why not test with both everywhere? For example, in "late-math-lowering.f90"? No worries if this is just an commission (could you update accordingly?), but if this is deliberate then it would be good to document the rationale for using only one driver.

Question 4.
AFAIK, fast, relaxed and precise are names from pgmath. I was under the impression that in #1377 we agreed to converge towards Clang's -ffp-model=<strict|precise|fast>. Like @kiranchandramohan points out, folks in Clang took some time to unify and to clarify their design in this area. I think that we should either aim for consistency with Clang. If it's decided otherwise, it would be good to clarify the rationale for diverging from that.

Thanks for taking a look,
Andrzej

vzakhari updated this revision to Diff 439457.Jun 23 2022, 10:26 AM

+1. Yes, that would be a fast way to get that portion of the patch in.

Thanks. I extracted the math change into https://reviews.llvm.org/D128454

This is really great to see. I like using the MLIR math dialect operations and it also provides a pathway to not depend on external libraries.

Also, is this a one-off patch or do you plan to work further on this? Would we have to extend the math dialect to handle all the fortran math intrinsics? Have you explored using the complex dialect operations directly, like e.g. https://mlir.llvm.org/docs/Dialects/ComplexOps/#complexsin-mlircomplexsinop?

Correct, I am going to work further on this and using complex dialect is the next step. The intention of this initial patch is to agree on the direction of using more MLIR operations, which should enable more MLIR optimizations.

Regarding adding MLIR operations for Fortran intrinsics or other math "primitives" that Fortran intrinsics expand into, I think we have to decide this case by case. I suppose there might be some math operations for which that we do not expect many optimizations happen right now. For example, I do not expect any optimizations for a group of BESSEL intrinsics currently, so it may not make much sense to introduce math operations for them right now. We may reconsider this in future, when math dialect optimizations (e.g.) support more operations. Of course, there are other optimizations that may be applied even to BESSEL intrinsics, e.g. CSE, but I would like to enable it via the means of setting proper SideEffects for the libm calls (which is TBD: e.g. libm's j0 may have no side effects on FP control/status word unless FP strict mode is requested, which may enable CSEing j0 calls under fast). So I would expect that some operations are going to be represented as libm calls for a while.

Hi @awarzynski,

Thank you for reviewing!

Hi @vzakhari !

Thanks for working on this. Before diving into details, I have a couple of design questions.

Question 1.
What's the end goal here? I can see how this could allow us to drop the dependency on libpgmath. Is that what you are aiming here for? What's the intended workflow for end users? We've discussed this before, see #1377. It would be good to clarify what points from that discussion are being addressed here.

The initial goal was to expose as many MLIR operations as possible for the purpose of enabling more optimizations. There are existing math dialect optimizations and there will be more going forward, so lowering to MLIR operations seems like a good idea. Moreover, the used MLIR operations have no side effects comparing to generic library calls, so using MLIR operations seems to be a good way for enabling generic optimizations like CSE, LICM, etc. (though, we have to keep in mind that FP strict does imply side effects for math operations and this is not currently modelled in MLIR, so generating MLIR operations may be considered a "regression" comparing to generic library calls under FP strict).

With that said, I did not plan to generate libm calls instead of pgmath calls, but after discussion with the team it seemed like a good opportunity to also get rid of pgmath dependency under late math lowering option. This should simplify flang toolchain setup for the upstream users.

Question 2.

Added new -math-lowering option that defaults to 'early' that matches the current math lowering scheme.

Could you clarify what you mean by this? There are 4 drivers in Flang (bbc, tco, flang-new and flang-new -fc1). From what I can see, you've used the llvm::cl API, but flang-new and flang-new -fc1 options are defined in Clang's Options.td. I've noticed that you've used -mmlir to forward these options, but -mmlir is intended for developers rather than end-users. So what would be the path for enabling this for end users? If this effort is to remove the dependency on libpgmath, then I think that it would make sense to make this available sooner rather than later.

Right, this commit only adds initial support for lowering into MLIR math operations or libm calls. I did not want to expose user option(s) to select late mode before confirming that the proposed change seems reasonable/useful to everyone. For the time being, I use -mllvm to enable this "experimental" mode, but I think it will have to become default and the pgmath dependency should go away completely (at least in the lowering implementation). Note that pgmath dependency may still be optional, e.g. for vectorizing libm calls into pgmath vector calls as in here (see TargetLibraryInfoImpl::addVectorizableFunctionsFromVecLib() changes in lib/Analysis/TargetLibraryInfo.cpp).

To summarize, I expect that we will enable late lowering by default and an option will not be needed.

Question 3.
It's good to see that you are testing with both flang-new and bbc, but why not test with both everywhere? For example, in "late-math-lowering.f90"? No worries if this is just an commission (could you update accordingly?), but if this is deliberate then it would be good to document the rationale for using only one driver.

Thank you for pointing it out! I do not see any problems with testing it with both drivers, so I will make changes.

Question 4.
AFAIK, fast, relaxed and precise are names from pgmath. I was under the impression that in #1377 we agreed to converge towards Clang's -ffp-model=<strict|precise|fast>. Like @kiranchandramohan points out, folks in Clang took some time to unify and to clarify their design in this area. I think that we should either aim for consistency with Clang. If it's decided otherwise, it would be good to clarify the rationale for diverging from that.

Thank you for bringing up this point! Yes, I agree that it makes great sense to align flang behavior with clang, and -ffp-model is the right option to control how MLIR operations and the libm calls are handled in the compiler.

I believe the different versions of pgmath can be described as:

  • precise - matches libm functions behavior (e.g. matching errno, exception behavior). With regards to math library calls it seems to match clang's -ffp-model=strict.
  • fast - matching accuracy for scalar and vector versions; does not set errno; exception behavior may not match precise in some cases. I guess we can say that it matches -ffp-model=fast, but I am not 100% sure: e.g. I think llvm.sqrt with Fast-Math Flag 'afn' (which is set under -ffp-model=fast) will be less accurate than pgmath's fast sqrt call. So maybe -ffp-model=fast actually matches pgmath's relaxed, and pgmath's fast matches -ffp-model=fast with some Fast-Math Flags disabled (which may be controlled by clang's other options).
  • relaxed - more inaccurate than fast; no specified accuracy for any entry point; scalar and vector versions may provide different accuracy.

If we switch to libm calls, I think it makes sense to align with clang completely, i.e.:

  • -ffp-model=fast - use math/complex MLIR operations or libm calls (if there is no corresponding MLIR operation) with all MLIR Fast-Math Flags set. Note that the 'afn' flag should probably enable "unsafe" optimizations such as math polynomial approximations. To avoid too much inaccuracy, users will be able to specify -fno-approx-func.
  • -ffp-model=strict - until MLIR operations are able to model FP strict behavior, I guess, we will have to generate generic library calls for all math operations and hope that the optimizations do not violate it too much. Note that we are not honoring FP strict behavior not only for math function, but for user calls as well, e.g. LLVM IR uses strictfp call attribute to control the inlining, and there is nothing like that in MLIR currently.
  • -ffp-model=precise - it must support errno and IEEE denormal-fp-math which is currently not modelled in MLIR, so at the current point in time we can treat it the same as -ffp-mode=strict.

So my current changes are just trying to match the existing precise/fast/relaxed behavior for pgmath, and more work will be needed to properly follow clang's -ffp-model within MLIR. I will be glad to hear your thoughts on this topic. FYI, I think SideEffectsInterface is one of the ways to model fenv_access and -ffp-exception-behavior=strict, but we also need to model the rounding mode and denormal behavior in MLIR.

Thanks for taking a look,
Andrzej

vzakhari updated this revision to Diff 439532.Jun 23 2022, 2:09 PM
clementval accepted this revision.Jun 24 2022, 12:06 AM

Thanks for looking into this. Looks good.

Hi @vzakhari, thank you for you very comprehensive reply and taking the time to address my question, much appreciated!

I've left a few inline comments, but that's mostly asking for clarifications.

I've noticed that in "late-math-lowering.f90" you test lowering and in "late-math-codegen.f90" you test lowering + code-gen. Since "late-math-lowering.f90" is there to make sure that lowering works as expected, why not focus on code-gen in "late-math-codegen.f90"? Wouldn't that make future triaging easier? Otherwise, any failure in "late-math-codegen.f90" could originate from 2 different compiler stages (either lowering or code-gen).

More comments below.

With that said, I did not plan to generate libm calls instead of pgmath calls, but after discussion with the team it seemed like a good opportunity to also get rid of pgmath dependency under late math lowering option. This should simplify flang toolchain setup for the upstream users.

+1 Great to see this being included :) In my view, it would be good to advertise this a bit (perhaps on Discourse). It's a non-trivial design change that will affect people. I don't expect anyone to oppose, but we should strive to make this discussion inclusive. It doesn't have to happen immediately - you may want to wait until there's enough support to drop the pgmath dependency.

Right, this commit only adds initial support for lowering into MLIR math operations or libm calls. I did not want to expose user option(s) to select late mode before confirming that the proposed change seems reasonable/useful to everyone. For the time being, I use -mllvm to enable this "experimental" mode, but I think it will have to become default and the pgmath dependency should go away completely (at least in the lowering implementation).

That's fine, but it wasn't clear from the summary :) This is quite an important functional change and folks might interested to learn more about. I think that it's worth pointing all this out in the commit msg.

Note that pgmath dependency may still be optional, e.g. for vectorizing libm calls into pgmath vector calls as in here (see TargetLibraryInfoImpl::addVectorizableFunctionsFromVecLib() changes in lib/Analysis/TargetLibraryInfo.cpp).

I agree that we should allow users to select their preferred Math library. Probably through a compiler driver flag.

To summarize, I expect that we will enable late lowering by default and an option will not be needed.

+1 Thanks for clarifying!

Question 4.
AFAIK, fast, relaxed and precise are names from pgmath. I was under the impression that in #1377 we agreed to converge towards Clang's -ffp-model=<strict|precise|fast>. Like @kiranchandramohan points out, folks in Clang took some time to unify and to clarify their design in this area. I think that we should either aim for consistency with Clang. If it's decided otherwise, it would be good to clarify the rationale for diverging from that.

Thank you for bringing up this point! Yes, I agree that it makes great sense to align flang behavior with clang, and -ffp-model is the right option to control how MLIR operations and the libm calls are handled in the compiler.

Great!

If we switch to libm calls

Does libm implement all Math functions that we need? Also, perhaps we should discuss the mapping between FP modes/options in a separate thread?

Thank you,
-Andrzej

flang/lib/Lower/IntrinsicCall.cpp
964–965

[nit] The command line option is defined on line 991 rather than 981.

968–970

Basically, the mathRuntimeVersion generation will happen for these math operations late during FIR conversion.

It won't happen late, right? It will happen either late or early and that will be determined by the value of mathLowering.

972–980

I feel that this comment mixes overall justification for the approach taken here _with_ the documentation for what MathLoweringMode represents. For example:

In order to preserve strict FP behavior with late math lowering we have to extend the dialects used by the late lowering such that they model strict FP behavior properly.

I think that this is a generic design challenge here that's orthogonal to the meaning of MathLoweringMode. Would you mind expanding/splitting this a bit? Easier said than done, I know!

982–983

I'm a bit confused. In your tests you use -math-lowering=late -mllvm -math-runtime=precise, which implies that mathRuntimeVersion applies to both earlyLowering and lateLowering. But this comment suggests that mathRuntimeVersion is only relevant for earlyLowering.

986–987

[nit] lateLowering suggests that this defines when Math intrinsics are going to be lowered. But the comment focus on what Math intrinsics are going to be lowered to instead of when it's going to happen.

1096
1160–1162

[nit] "Map mathematical intrinsic operations into MLIR operations" is a bit misleading - "map" is a verb, but this is a variable that represents state and I would expect a noun.

1163

[nit] Rather than saying "more", could you clarify where to look for the complete list? Or perhaps "remaining Fortran Math intrinsics"?

1226

Why can't it be removed now?

1458–1459

Could you add doxygen?

1484

Function names should be verb phrases (as they represent actions), and command-like function should be imperative.

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

1980

So this is effectively earlyLowering, right? Which will also happen if lateLowering fails?

It would be nice if you could lower most of the Fortran math intrinsics to the Math dialect. You can extend it as needed. LLVM backends can then decide whether they need libcalls.

It should give you some optimisation opportunities in MLIR.

Hi @awarzynski,

Thank you for the thorough review! Please see my comments below.

Hi @vzakhari, thank you for you very comprehensive reply and taking the time to address my question, much appreciated!

I've left a few inline comments, but that's mostly asking for clarifications.

I've noticed that in "late-math-lowering.f90" you test lowering and in "late-math-codegen.f90" you test lowering + code-gen. Since "late-math-lowering.f90" is there to make sure that lowering works as expected, why not focus on code-gen in "late-math-codegen.f90"? Wouldn't that make future triaging easier? Otherwise, any failure in "late-math-codegen.f90" could originate from 2 different compiler stages (either lowering or code-gen).

Sounds reasonable to me. I changed the test.

More comments below.

With that said, I did not plan to generate libm calls instead of pgmath calls, but after discussion with the team it seemed like a good opportunity to also get rid of pgmath dependency under late math lowering option. This should simplify flang toolchain setup for the upstream users.

+1 Great to see this being included :) In my view, it would be good to advertise this a bit (perhaps on Discourse). It's a non-trivial design change that will affect people. I don't expect anyone to oppose, but we should strive to make this discussion inclusive. It doesn't have to happen immediately - you may want to wait until there's enough support to drop the pgmath dependency.

Sounds good. I will post a message on Discourse, when late lowering is sound. Is there any particular "channel" on Discourse or should I use special tags?

Right, this commit only adds initial support for lowering into MLIR math operations or libm calls. I did not want to expose user option(s) to select late mode before confirming that the proposed change seems reasonable/useful to everyone. For the time being, I use -mllvm to enable this "experimental" mode, but I think it will have to become default and the pgmath dependency should go away completely (at least in the lowering implementation).

That's fine, but it wasn't clear from the summary :) This is quite an important functional change and folks might interested to learn more about. I think that it's worth pointing all this out in the commit msg.

I will update the commit message.

Note that pgmath dependency may still be optional, e.g. for vectorizing libm calls into pgmath vector calls as in here (see TargetLibraryInfoImpl::addVectorizableFunctionsFromVecLib() changes in lib/Analysis/TargetLibraryInfo.cpp).

I agree that we should allow users to select their preferred Math library. Probably through a compiler driver flag.

To summarize, I expect that we will enable late lowering by default and an option will not be needed.

+1 Thanks for clarifying!

Question 4.
AFAIK, fast, relaxed and precise are names from pgmath. I was under the impression that in #1377 we agreed to converge towards Clang's -ffp-model=<strict|precise|fast>. Like @kiranchandramohan points out, folks in Clang took some time to unify and to clarify their design in this area. I think that we should either aim for consistency with Clang. If it's decided otherwise, it would be good to clarify the rationale for diverging from that.

Thank you for bringing up this point! Yes, I agree that it makes great sense to align flang behavior with clang, and -ffp-model is the right option to control how MLIR operations and the libm calls are handled in the compiler.

Great!

If we switch to libm calls

Does libm implement all Math functions that we need?

Good question. No, libm is definitely missing some functions, e.g. there is no pow with integer exponent. There is a set of powi* functions in libgcc, but I do not think we can rely on this. We may have PowI operation in math and complex dialects, but then we have to lower it into LLVM IR dialect somehow. One of the solutions is to have all needed powi versions in Fortran numeric runtime and generate calls to them late in codegen.

Also, perhaps we should discuss the mapping between FP modes/options in a separate thread?

Yes, the options mapping and (arith, math, complex, etc.) MLIR operations support for different FP modes need to be discussed so that we can match user expectations regarding FP behavior.

Thank you,
-Andrzej

flang/lib/Lower/IntrinsicCall.cpp
968–970

You are correct. This (and other comments) is a leftover from the initial implementation that tried to convert MLIR operations into pgmath calls late in codegen. I will fix it.

972–980

Right, I think I will just remove this comment and we will discuss the general issue on Discourse.

982–983

Right, mathRuntimeVersion applies to both early and late lowering.

986–987

I guess the options should be renamed to something else, but I cannot come up with a good name :)

1160–1162

Fixed.

1163

Fixed.

1226

It is still used in the early mode both under -math-runtime=llvm and under any other -math-runtime if the corresponding operation is not found in pgmath list in flang/include/flang/Evaluate/pgmath.h.inc.

1458–1459

Done.

1484

Renamed to checkPrecisionLoss.

1980

Exactly. The late lowering falls back to early lowering currently.

It would be nice if you could lower most of the Fortran math intrinsics to the Math dialect. You can extend it as needed. LLVM backends can then decide whether they need libcalls.

It should give you some optimisation opportunities in MLIR.

@tschuett, thank you for the comment. In general, I agree with you, but please see my comment regarding BESSEL functions here: https://reviews.llvm.org/D128385#3605776
I think having MLIR operations may not make much sense for math functions, which have no math-specific optimization opportunities. As I said before, if we can model SideEffects for generic calls such that, for example, a math call has NoSideEffect under -ffp-model=fast, then non-math-specific optimizations (e.g. CSE, LICM) should just start working.

vzakhari updated this revision to Diff 439858.Jun 24 2022, 12:43 PM
vzakhari edited the summary of this revision. (Show Details)

Thanks for all the updates, @vzakhari !

Is there any reason not to split your tests in "late-math-lowering.f90" and "late-math-codegen.f90" into smaller testable units? Similarly to e.g. convert-to-llvm-ir.fir?

  • With the way the tests are written right now, it's hard to match the input with the output (both are very long). This becomes much easier when testing one intrinsic at a time (i.e. there's a separate function to test a particular intrinsic call).
  • By splitting the tests, you could also leverage CHECK-SAME and CHECK-LABEL more. This would help verifying that a particular intrinsic receives correct results.
  • Also, the current approach makes adding new test rather hard? More specifically, this is a very complex test input for somebody who is only interested in the abs intrinsic:
function test_real4(x, y, c, s, i)
  real :: x, y, test_real4
  complex(4) :: c
  integer(2) :: s
  integer(4) :: i
  test_real4 = abs(x) + abs(c) + aint(x) + anint(x) + atan(x) + atan2(x, y) + &
       ceiling(x) + cos(x) + erf(x) + exp(x) + floor(x) + log(x) + log10(x) + &
       nint(x, 4) + nint(x, 8) + x ** s + x ** y + x ** i + sign(x, y) + &
       sin(x) + tanh(x)
end function

Would you be open to refactoring the tests to match the style adapted in other test files? More comments inline.

-Andrzej

flang/lib/Lower/IntrinsicCall.cpp
986–987

Naming is hard!

Since there are only two values, you may want to change this to a bool option instead, e.g.:

static llvm::cl::opt<bool> lowerEarlyToLibCall("lower-early", llvm::cl::desc("Controls when to lower Math intrinsics to library calls"), llvm::cl::init(true));

That would probably simplify the code and communicate the overall intent a bit better. And with only one option to document, you will require fewer comments :)

1072

Shouldn't this be genF64IntF64FuncType instead of genF64F64IntFuncType? As in, the arguments are F64 and Int and the return type is F64. Same for genF32F32IntFuncType.

1099–1100

[nit] Perhaps runtimeFunc so that the intent is clearer?

1103

Where is this condition checked? (i.e. if (!funcGenerator) or something similar).

Also, I'm a bit confused about the relationship between symbol and funcGenerator. To me this comment suggests that either symbol or funcGenerator is used, but in fact funcGenerator will use symbol to generate the correct call:

result = mathOp->funcGenerator(builder, loc, mathOp->symbol, actualFuncType, convertedArguments);

Could you clarify?

1110–1123

Does my suggestion make sense? Perhaps I'm confusing things here. My point is that name is quite enigmatic and it's tricky to follow this function without more descriptive names. Same comment for genMathOp.

1126–1128
1226

Thanks!

[nit] it would be helpful to have a comment like this here :) (basically, so that we know when to delete this)

@awarzynski, thank you for the review!

I modified the tests so that each split part tests particular Fortran operation. I will upload it shortly. Please also see my replies to your comments inlined.

flang/lib/Lower/IntrinsicCall.cpp
1072

I followed the same convention as in genIntF32FuncType above: the first type is the result type, and the following types are the argument types, so genF64F64IntFuncType stands for F64 (*)(F64, Int<Bits>), which seems to be a straightforward mapping from the function type (as writtent in C) and the type generator name.

1099–1100

Fixed.

1103

Yes, the comment does not make sense any more. I fixed it. Thanks for catching!

1110–1123

Your suggested code looks good to me. I applied it.

vzakhari updated this revision to Diff 440438.Jun 27 2022, 5:05 PM
vzakhari edited the summary of this revision. (Show Details)
vzakhari updated this revision to Diff 440651.Jun 28 2022, 9:11 AM

Latest revision is rebase + Lower/array-expression-slice-1.f90 fix.

Thanks again for working on this. In particular, for refactoring the tests. It would be nice if you could reduce them a bit (specific suggestions inline). I won't have any more comments after that!

Btw, I finally had a chance to go over the most interesting part (thanks for writing this down!):

Thank you for bringing up this point! Yes, I agree that it makes great sense to align flang behavior with clang, and -ffp-model is the right option to control how MLIR operations and the libm calls are handled in the compiler.

I believe the different versions of pgmath can be described as:

  • precise - matches libm functions behavior (e.g. matching errno, exception behavior). With regards to math library calls it seems to match clang's -ffp-model=strict.
  • fast - matching accuracy for scalar and vector versions; does not set errno; exception behavior may not match precise in some cases. I guess we can say that it matches -ffp-model=fast, but I am not 100% sure: e.g. I think llvm.sqrt with Fast-Math Flag 'afn' (which is set under -ffp-model=fast) will be less accurate than pgmath's fast sqrt call. So maybe -ffp-model=fast actually matches pgmath's relaxed, and pgmath's fast matches -ffp-model=fast with some Fast-Math Flags disabled (which may be controlled by clang's other options).
  • relaxed - more inaccurate than fast; no specified accuracy for any entry point; scalar and vector versions may provide different accuracy.

If we switch to libm calls, I think it makes sense to align with clang completely, i.e.:

  • -ffp-model=fast - use math/complex MLIR operations or libm calls (if there is no corresponding MLIR operation) with all MLIR Fast-Math Flags set. Note that the 'afn' flag should probably enable "unsafe" optimizations such as math polynomial approximations. To avoid too much inaccuracy, users will be able to specify -fno-approx-func.
  • -ffp-model=strict - until MLIR operations are able to model FP strict behavior, I guess, we will have to generate generic library calls for all math operations and hope that the optimizations do not violate it too much. Note that we are not honoring FP strict behavior not only for math function, but for user calls as well, e.g. LLVM IR uses strictfp call attribute to control the inlining, and there is nothing like that in MLIR currently.
  • -ffp-model=precise - it must support errno and IEEE denormal-fp-math which is currently not modelled in MLIR, so at the current point in time we can treat it the same as -ffp-mode=strict.

So my current changes are just trying to match the existing precise/fast/relaxed behavior for pgmath, and more work will be needed to properly follow clang's -ffp-model within MLIR. I will be glad to hear your thoughts on this topic. FYI, I think SideEffectsInterface is one of the ways to model fenv_access and -ffp-exception-behavior=strict, but we also need to model the rounding mode and denormal behavior in MLIR.

I've not investigated the precise meaning of these flags myself - sadly that's always been on a back-burner. Your summary makes a lot of sense to me - thanks for taking all this into consideration while working on this patch. I guess that we can dive into the fine details once follow-up patches are up for review?

-Andrzej

flang/lib/Lower/IntrinsicCall.cpp
1072

Makes sense, thanks for the explanation!

1102

Did I get this right?

1105

Did I get this right?

flang/test/Intrinsics/late-math-codegen.fir
15

Remove unused args. Similar suggestion for other tests.

flang/test/Lower/late-math-lowering.f90
24

hypotf is for abs(c), right? Why not move it to a dedicated function/test? (e.g. test_complex(c))

26–32

I would remove the code that's not needed for this particular test. Similar suggestion for other tests.

clementval accepted this revision.Jun 28 2022, 10:11 AM

Still LGTM

Thank you all for the reviews! I will upload the final changes shortly and merge them after pre-merge checks pass.

flang/lib/Lower/IntrinsicCall.cpp
1102

Thanks! Fixed.

1105

Thanks!

flang/test/Intrinsics/late-math-codegen.fir
15

Fixed.

flang/test/Lower/late-math-lowering.f90
24

Makes sense. Fixed.

26–32

Fixed.

vzakhari updated this revision to Diff 440691.Jun 28 2022, 10:40 AM
vzakhari edited the summary of this revision. (Show Details)
awarzynski accepted this revision.Jun 28 2022, 11:26 AM

LGTM, many thanks! Could you give @kiranchandramohan a chance to give his blessing too? (i.e. could you wait another day before merging?)

LGTM, many thanks! Could you give @kiranchandramohan a chance to give his blessing too? (i.e. could you wait another day before merging?)

You can go ahead. I am unlikely to get a chance to have a look today or tomorrow.

flang/lib/Lower/IntrinsicCall.cpp
969

Nit: know -> known

vzakhari retitled this revision from Lower Fortran math intrinsic operations into MLIR ops or libm calls. to [flang] Lower Fortran math intrinsic operations into MLIR ops or libm calls..Jun 28 2022, 1:25 PM
vzakhari edited the summary of this revision. (Show Details)
vzakhari added inline comments.
flang/lib/Lower/IntrinsicCall.cpp
969

Thanks. Fixed.

Feel free to post more comments after the merge.