Page MenuHomePhabricator

Fix FREM on 32-bit MSVC on x86

Authored by dylanmckay on Aug 18 2015, 2:28 AM.



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

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.

Diff Detail


Event Timeline

dylanmckay updated this revision to Diff 32393.Aug 18 2015, 2:28 AM
dylanmckay retitled this revision from to Fix FREM on 32-bit MSVC on x86.
dylanmckay updated this object.
dylanmckay updated this object.

Checking if llvm-commits is subscription is working yet.

mbodart added inline comments.Aug 18 2015, 8:24 AM
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 updated this object.Aug 18 2015, 8:25 AM
dylanmckay added inline comments.Aug 18 2015, 8:37 AM
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 updated this revision to Diff 32417.Aug 18 2015, 8:51 AM

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

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

dylanmckay marked 2 inline comments as done.Aug 18 2015, 8:51 AM
tamird added a subscriber: tamird.Aug 18 2015, 9:28 AM

Hi Dylan,

The changes LGTM.

  • mitch

Cheers. Can somebody commit this?

This revision was automatically updated to reflect the committed changes.
vsk added a subscriber: vsk.Sep 1 2015, 6:35 PM

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