This is an archive of the discontinued LLVM Phabricator instance.

Lowering and runtime support for F08 transformational intrinsics: BESSEL_JN and BESSEL_YN
ClosedPublic

Authored by tarunprabhu on Dec 5 2022, 11:27 AM.

Details

Summary

This patch implements lowering and adds runtime support for F08 transformational variants of BESSEL_JN and BESSEL_YN.

The runtime implementation uses the recurrence relations

`J(n-1, x) = (2.0 / x) * n * J(n, x) - J(n+1, x)`
`Y(n+1, x) = (2.0 / x) * n * Y(n, x) - Y(n-1, x)`

(see https://dlmf.nist.gov/10.74.iv and https://dlmf.nist.gov/10.6.E1).

The special case for x == 0 in BESSEL_JN(N1, N2, x) and BESSEL_YN(N1, N2, x) are handled in the same runtime function although the check for x == 0 is carried out in FIR. Similarly, the anchor points for the recursion are also calculated in FIR and passed explicitly to the runtime function.

Tests have been added for the new runtime functions and the lowering.

The existing tests for lowering were modified so the tests for the elemental and transformational variants are stylistically more consistent.

Possible alternatives:

  • Pass the elemental Bessel function that is used to calculate the anchor points of the recursion to the runtime instead of calculating it in FIR.
  • Split the runtime implementation for the x == 0 and x != 0 cases.

Diff Detail

Event Timeline

tarunprabhu created this revision.Dec 5 2022, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 11:27 AM
tarunprabhu requested review of this revision.Dec 5 2022, 11:27 AM
vzakhari added inline comments.Dec 8 2022, 10:46 PM
flang/lib/Lower/IntrinsicCall.cpp
2695

I think unconditional computation of the recursion bases might be suboptimal at least for the n1 == n2 case. Can you please rewrite the selects with IfOp?

flang/lib/Optimizer/Builder/Runtime/Transformational.cpp
31

For portability, I think it is better to use 32 here and int32_t in the runtime implementation.

flang/runtime/transformational.cpp
168

Should we also verify that n1 and n2 are non-negative?

193

Early return is more preferable.

tarunprabhu added inline comments.Dec 9 2022, 8:36 AM
flang/runtime/transformational.cpp
168

The Fortran standard specifies that n1 and n2 should be non-negative.

However, gfortran and ifort do not seem to check for this case. The output of this patch as currently implemented is comparable to the behavior of those two compilers when x > 0.0. The x == 0.0 case is a bit messier.

A similar concern arose with one of the other intrinsics (https://reviews.llvm.org/D129316#3638582. See also @klausler's comment in the discourse post linked there). As before, I am happy to adhere more closely to the standard, or be more compatible with other compilers. For the latter case, I could do as @jeanPerier suggested in that patch and document our implementation more clearly.

However, we do diverge from other compilers in the behavior of the intrinsics when compile-time constant negative values are passed. Both gfortran and ifort raise a compile-error in that case, while we do not. The runtime implementation is consistent with the current behavior when the parameters are compile-time constants.

Significant changes to the patch:

  • Use fir::IfOp so anchor points for the recursion are only calculated if necessary.
  • Created runtime functions specialized for the x == 0 case. This has cleaned up the signature of the runtime functions so only those arguments that are necessary are passed along.
  • Use int32_t in the signature for the runtime functions instead of relying on the width of the system's int datatype.
  • Added tests for the genBessel* lowering functions in addition to tests of the runtime functions themselves.
tarunprabhu marked 3 inline comments as done.Dec 9 2022, 1:52 PM

Thank you for the explanantion and the changes! We can assert that n1 and n2 are non-negative later, if we decide to be more verbose in runtime.

flang/unittests/Runtime/Transformational.cpp
343

Can you please add a unit test for BesselYnX0_16? We've had issues before that std::numeric_limits was not fully implemented for CppTypeFor<TypeCategory::Real, 16> (e.g. D134496).

  • Added comments in the runtime functions indicating that the non-negativity constraint on n1 and n2 that is required by the standard is not enforced.
  • Modified the runtime tests so all kinds (4, 8, 10 and 16) are now tested (with appropriate guards for the KIND=10 and KIND=16 cases).
  • Fixed includes in transformational.h and fixed the signature of some of the functions which were incorrect.
tarunprabhu marked an inline comment as done.Dec 13 2022, 12:51 PM
vzakhari accepted this revision.Dec 14 2022, 6:28 PM

Thank you, Tarun!

This revision is now accepted and ready to land.Dec 14 2022, 6:28 PM
jeanPerier accepted this revision.Dec 19 2022, 12:58 AM
vzakhari added inline comments.Jan 17 2023, 9:04 AM
flang/lib/Lower/IntrinsicCall.cpp
2682

Hello @tarunprabhu, is unordered predicate here intentional?

tarunprabhu added inline comments.Jan 17 2023, 10:16 AM
flang/lib/Lower/IntrinsicCall.cpp
2682

I think I intended this to replicate the behavior of gfortran in the case when x is NaN. I'll have to double-check that though.

vzakhari added inline comments.Jan 17 2023, 11:27 AM
flang/lib/Lower/IntrinsicCall.cpp
2682

Yes, please double-check it, because treating NaN argument as zero may not be the right thing to do. We'd better propagate the NaN argument to the result.