Skip to content

Constant folding for 'frem' is flaky #3688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
llvmbot opened this issue Jan 12, 2009 · 12 comments
Closed

Constant folding for 'frem' is flaky #3688

llvmbot opened this issue Jan 12, 2009 · 12 comments
Labels
bugzilla Issues migrated from bugzilla llvm:core

Comments

@llvmbot
Copy link
Member

llvmbot commented Jan 12, 2009

Bugzilla Link 3316
Resolution FIXED
Resolved on Apr 19, 2017 14:20
Version trunk
OS MacOS X
Reporter LLVM Bugzilla Contributor
CC @RKSimon,@sanjoy

Extended Description

Constant folding for 'frem' seems flaky for certain values. For example, "frem float 0.1, 1.0" returns -0.9 when constant folded:

define float @​test1() {
entry:
%y = frem float 0x3FB99999A0000000, 1.0
ret float %y
}

@​x = global float 0x3FB99999A0000000 ; float 0.1

define float @​test2() {
%x = load float* @​x
%y = frem float %x, 1.0
ret float %y
}

@lattner
Copy link
Collaborator

lattner commented Jan 12, 2009

I believe that frem is implemented in terms of the fmod libm function. Does your libm produce this result?

@llvmbot
Copy link
Member Author

llvmbot commented Jan 12, 2009

Nope, fmodf() and fmod() both behave normally (gcc 4.0.1 under Mac OS 10.5.5), and frem works fine when it's not constant folded. (In the example I included, the second function returns the correct value.)

  • Mark

@lattner
Copy link
Collaborator

lattner commented Jan 19, 2009

This may be a ConstantFP bug. We constant fold frem (which is the same as libm fmod) with:

   (void)C3V.mod(C2V, APFloat::rmNearestTiesToEven);

@llvmbot
Copy link
Member Author

llvmbot commented Jan 19, 2009

It's in APFloat::convertToSignExtendedInteger, which is converting 0.1 to 1 at rounding mode NearestTiesToEven. Looks like it's been there since the beginning. Surprising this hasn't shown up before; is mod really the only thing that uses this?

@llvmbot
Copy link
Member Author

llvmbot commented Jan 19, 2009

While what I said above is true, a more direct cause is that the conversion in APFloat::mod should be using RoundToZero.

@llvmbot
Copy link
Member Author

llvmbot commented Jan 19, 2009

I see, currently the FP-to-integer conversions are always called with TowardsZero, except this one place. So the bug calling it with NearestTiesToEven would never show up anywhere else. This fixes both bugs.
Neil, please review.
http://llvm.org/viewvc/llvm-project?view=rev&revision=62528

@lattner
Copy link
Collaborator

lattner commented Jan 19, 2009

Nice, thanks Dale!

@llvmbot
Copy link
Member Author

llvmbot commented Jan 20, 2009

OK, looking at this further, I've (re)discovered that IEEE remainder does use round-to-nearest, and is therefore not equivalent to C fmod. To confuse matters further, llvm docs refer to the C fmod operation as "remainder" (hence llvm frem is equivalent to C fmod) and the IEEE remainder operation as "modulo". It appears the intent of APFloat::mod was to implement IEEE remainder (although I don't see that stated anywhere) and I have just broken it (further).
I think the best thing to do is hijack APFloat::mod for the C fmod operation, which (a) fits the name better and (b) is much more useful in the context of llvm. IEEE remainder would be a nice thing to have but I don't think there are any clients for it currently. I am not very knowledgeable about Fortran and Ada though...

@llvmbot
Copy link
Member Author

llvmbot commented Jan 31, 2017

I'm seeing a similar problem, though for different constant values, in LLVM 3.9.1:

$ cat frem.ll
define double @​test1() {
entry:
%y = frem double 3.000000e-01, 1.000000e-02
ret double %y
}

Compiled without optimisations gives the correct answer:

$ llc frem.ll && ./frem
0x1.47ae147ae1471p-7

However applying a constprop pass gives an incorrect answer:

$ opt -S -constprop frem.ll
; ModuleID = 'frem.ll'
source_filename = "frem.ll"

define double @​test1() {
entry:
ret double 0.000000e+00
}

downstream issue: JuliaLang/julia#14089

@llvmbot
Copy link
Member Author

llvmbot commented Jan 31, 2017

Ignore that llc line, but the rest remains correct. Another error, though presumably from a different code path (0x43E0000000000000 is 0x1p63)

$ cat frem2.ll
define double @​test1() {
entry:
%y = frem double 0x43E0000000000000, 1.000000e+00
ret double %y
}

$ opt -S -constprop frem2.ll
; ModuleID = 'frem2.ll'
source_filename = "frem2.ll"

define double @​test1() {
entry:
ret double 0x43E0000000000000
}

(answer should be 0 here)

@sanjoy
Copy link
Contributor

sanjoy commented Mar 8, 2017

32177 is most likely a duplicate of this bug. If 32177 fixes itself after D29346 goes in, I'll mark it as DUPLICATE.

@RKSimon
Copy link
Collaborator

RKSimon commented Apr 19, 2017

D29346 was committed at rL299256

Added remaining test from this ticket at rL300757

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla llvm:core
Projects
None yet
Development

No branches or pull requests

4 participants