Fix FREM on 32-bit MSVC on x86

[dylanmckay]

Summary

The x86 MSVC CRT library does not contain a fmodf(f32) function. It
only has the 64-bit fmod(f64) function.

Inside the MSVC CRT C headers, fmodf(f32) is defined as an inline
function which casts the f32 into an f64 and calls fmod(f64),
casting the result back. The fmodf function is never emitted into the
CRT lib.

This patch fixes this by instead promoting a 32-bit FREM into a 64-bit
FREM, but only on a 32-bit MSVC target.

This fixes an undefined symbol: fmodf error when linking on 32-bit
Windows.

The reason why this problem isn't major (or been fixed yet) is that many
LLVM frontends do not have floating point modulus built-in (i.e. C/C++),
so users must explicitly make a call to them. This problem only arises when
a frem node is to be lowered, as LLVM must then choose the runtime library
function to call.

[dylanmckay]

Checking if llvm-commits is subscription is working yet.

[mbodart]

test/CodeGen/X86/frem-msvc32.ll

7 ↗

(On Diff #32393)

As written, this test will pass without your compiler changes as _fmod is a substring of _fmodf.

It needs to either incorporate CHECK-NOT fmodf, or use a regular expression that matches _fmod but not _fmodf. Perhaps this:

; CHECK: calll {{_fmod$}}

[dylanmckay]

test/CodeGen/X86/frem-msvc32.ll

7 ↗

(On Diff #32393)

I had trouble with this.

If you remove the -mtriple=i686-pc-windows-msvc flag to llc, fmodf is generated, and the test does fail as expected.

I tried CHECK-NOT: fmodf, which worked well, but I feel that the test should check that fmod is being called, as opposed to checking that any arbitrary function except fmodf is being called.

I like the regexp solution you give - it both makes sure that fmodf isn't being called, and fmod is. I will update the patch and use it - thanks!
I will also remove the calll instruction, as it makes the test unnecessarily brittle (what if LLVM selects a different function call instruction in the future) .

[dylanmckay]

Changed brittle CHECK: calll _fmod statement with a robust regexp.

Also added an empty line in the test description to improve readability.

[dylanmckay]

Gentle ping.

[mbodart]

Hi Dylan,

The changes LGTM.

• mitch

[dylanmckay]

Cheers. Can somebody commit this?

[vsk]

Thanks, committed rL246615: [CodeGen] Fix FREM on 32-bit MSVC on x86

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
   setOperationAction(ISD::SELECT_CC        , MVT::i32,   Expand);
   setOperationAction(ISD::SELECT_CC        , MVT::i64,   Expand);
   if (Subtarget->is64Bit())
     setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i32, Legal);
   setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i16  , Legal);
   setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i8   , Legal);
   setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i1   , Expand);
   setOperationAction(ISD::FP_ROUND_INREG   , MVT::f32  , Expand);
-  setOperationAction(ISD::FREM             , MVT::f32  , Expand);
+
+  if (Subtarget->is32Bit() && Subtarget->isTargetKnownWindowsMSVC()) {
+    // On 32 bit MSVC, `fmodf(f32)` is not defined - only `fmod(f64)`
+    // is. We should promote the value to 64-bits to solve this.
+    // This is what the CRT headers do - `fmodf` is an inline header
+    // function casting to f64 and calling `fmod`.
+    setOperationAction(ISD::FREM           , MVT::f32  , Promote);
+  } else {
+    setOperationAction(ISD::FREM           , MVT::f32  , Expand);
+  }
+
   setOperationAction(ISD::FREM             , MVT::f64  , Expand);
   setOperationAction(ISD::FREM             , MVT::f80  , Expand);
   setOperationAction(ISD::FLT_ROUNDS_      , MVT::i32  , Custom);
 
   // Promote the i8 variants and force them on up to i32 which has a shorter
   // encoding.
   setOperationAction(ISD::CTTZ             , MVT::i8   , Promote);
   AddPromotedToType (ISD::CTTZ             , MVT::i8   , MVT::i32);

llvm/trunk/test/CodeGen/X86/frem-msvc32.ll

+; Make sure that 32-bit FREM is promoted to 64-bit FREM on 32-bit MSVC.
+
+; MSVC does not have a 32-bit fmodf function, so it must be promoted to
+; a 64-bit fmod rtlib call.
+; RUN: llc -mtriple=i686-pc-windows-msvc -O0 < %s  | FileCheck %s
+
+; CHECK: @do_frem32
+; CHECK: {{_fmod$}}
+define float @do_frem32(float %a, float %b) {
+    %val = frem float %a, %b
+    ret float %val
+}