This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add platform-specific intrinsic lowering
AbandonedPublic

Authored by rovka on Sep 29 2022, 1:15 AM.

Details

Summary

Make it possible for math operations to be lowered in different ways on
different platforms. Use single precision hypot as a case study: on
most platforms, hypot can be lowered to hypotf. However, the MSVC
runtime is lacking a hypotf function and instead uses _hypotf (*).
This patch adds a platform check based on the FIR triple (which is
either the triple in the FIR module, or the system default triple) and
uses it to decide how to lower hypot.

Fixes https://github.com/llvm/llvm-project/issues/57563

(*) More specifically, MSVC's headers define hypotf as an inline
function that just calls _hypotf. This works fine for clang, since it
will include those headers, but flang only links with the CRT so we
don't get a free ride.

Diff Detail

Event Timeline

rovka created this revision.Sep 29 2022, 1:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
rovka requested review of this revision.Sep 29 2022, 1:15 AM
rovka added a comment.Sep 29 2022, 1:23 AM

I'm planning to add more tests (see TODO), but I thought it would be good to get some general feedback on the approach first. @Meinersbur had also suggested adding a hypotf to the runtime when compiling for Windows, but this approach seems more flexible and straightforward to me. Other opinions welcome, thanks!

flang/lib/Lower/IntrinsicCall.cpp
1248

Alternatively we could just set a default lambda that returns true, and then we wouldn't need a helper to check for nullptr. I wasn't sure if people would consider that simpler and if it's worth going through a call every time, as opposed to a nullptr check in most cases.

I think doing this target specific dispatch in lowering is a bit early (MLIR/FIR passes relying on recognizing libm functions would also need to special case based on the target. Maybe I am wrong here though @vzakhari or @MatsPetersson may have a better view).

I would rather suggest rewriting "hypotf" to "_hypotf" in https://github.com/flang-compiler/f18-llvm-project/blob/fir-dev/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp if possible, although I understand it might be easier/cleaner from an implementation point of view to do it in IntrinsicCall.cpp. The runtime solution would also sounds fine to me.

I think doing this target specific dispatch in lowering is a bit early (MLIR/FIR passes relying on recognizing libm functions would also need to special case based on the target. Maybe I am wrong here though @vzakhari or @MatsPetersson may have a better view).

I would rather suggest rewriting "hypotf" to "_hypotf" in https://github.com/flang-compiler/f18-llvm-project/blob/fir-dev/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp if possible, although I understand it might be easier/cleaner from an implementation point of view to do it in IntrinsicCall.cpp. The runtime solution would also sounds fine to me.

Yes, I think having the late rewrite is a more consistent approach. Consider cases when we generate a Math dialect operation that is then converted to a libm call - if the callee does not have "default" name of a platform, we will have the same issue, but it will have to be resolved somewhere around MathToLibm convertor. So it seems a better approach is to have a dedicated platform-specific convertor for libm calls that will be applied to MLIR regardless of where the libm calls are coming from.

rovka abandoned this revision.Sep 30 2022, 3:48 AM

Right, I see what you mean. I'll rework this, thanks!