Page MenuHomePhabricator

[InstCombine] miscompile of __builtin_fmod
AbandonedPublic

Authored by Quolyk on Nov 29 2017, 1:56 AM.

Details

Summary

Motivation: https://bugs.llvm.org/show_bug.cgi?id=34870
I'm totally not sure this is correct

Diff Detail

Event Timeline

Quolyk created this revision.Nov 29 2017, 1:56 AM
spatel edited edge metadata.

I don't know the history of the frem instruction in IR, and the description in http://llvm.org/docs/LangRef.html#frem-instruction is vague.

But based on the existing code, I think this is working as intended. If the instruction has the same semantics as the math library function 'fmod', then we do want to convert this to an IR instruction in clang.

Note that when I filed PR34870, I assumed the bug was in the IR optimizer in the instcombine pass, but if the IR instruction does not match the semantics of the math library function, then I'd be wrong. :)

Side note: I think there is a different bug here in clang because from what I can tell, we convert the builtin or libcall to 'frem' even when errno could be set by the call. D40044 doesn't address this case because frem is an LLVM instruction rather than an LLVM intrinsic.

efriedma edited edge metadata.Nov 29 2017, 12:22 PM

By many years of precedent, the "frem" instruction is supposed to match the C fmod(), as opposed to something else like the C99 remainder(); probably worth clarifying in LangRef.

Added a blurb to the LangRef with rL319437, so I think we can abandon this patch.

Added a blurb to the LangRef with rL319437, so I think we can abandon this patch.

Sounds good.

Quolyk abandoned this revision.Dec 14 2017, 10:28 PM