This is an archive of the discontinued LLVM Phabricator instance.

[flang] Rename hypotf on MSVC platforms
ClosedPublic

Authored by rovka on Oct 13 2022, 12:28 AM.

Details

Summary

The single precision hypot intrinsic is lowered to a call to the libm
hypotf function. However, the MSVC runtime lacks a hypotf function
and instead uses _hypotf (*). This patch tries to find and rewrite
calls to hypotf if we're on a MSVC platform.

Calls to libm functions can be introduced even after lowering (**).
Therefore, we try to do the rewriting at the very end of FIR to LLVM
lowering.

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.

(**) https://github.com/llvm/llvm-project/blob/56f94ede2af9a327e59fe84dbf8cbbb7bb1dfa79/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L3391

Diff Detail

Event Timeline

rovka created this revision.Oct 13 2022, 12:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2022, 12:28 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
rovka requested review of this revision.Oct 13 2022, 12:28 AM

Hi! This is a rework of https://reviews.llvm.org/D134860

@vzakhari Is this what you had in mind?

Also, should this functionality be in MLIR itself and only called here? I'm not very familiar with how MLIR is organized, so any thoughts here would be helpful :)

Sorry for the delay. It looks good to me in general.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
3617

Can you please combine this with the patterns/conversion above? I.e. add the MSVC conversion patterns into pattern and get rid of the separate applyPartialConversion below?

rovka updated this revision to Diff 469133.Oct 20 2022, 1:39 AM

Address review comments.
Thanks for the feedback :)

vzakhari accepted this revision.Oct 20 2022, 11:58 AM

Thanks! Looks good to me, though, if/when we have multiple such cases we will need to simplify the addition of function names and their mappings.

This revision is now accepted and ready to land.Oct 20 2022, 11:58 AM
rovka added a comment.Oct 21 2022, 1:35 AM

Thanks! Looks good to me, though, if/when we have multiple such cases we will need to simplify the addition of function names and their mappings.

I agree, I just didn't want to overengineer it just yet :) Thanks for the review!

This revision was automatically updated to reflect the committed changes.