LLVM 8 stopped optimizing pow(2., x) to exp2(x) #39887
Comments
|
Indeed, that patch fixed an issue when the call to exp2() would be emitted even when the run time environment didn't support it. LLVM assumes that Win32 only supports C89 math functions, which doesn't seem to be the case for a while anymore: https://is.gd/xFJPQw |
|
Is this something that could be safely fixed within the 8.0.0 release? |
|
I'm afraid that, as an enhancement, as you indicated above, it's too late for 8.0, since only bug fixes are being accepted now. My impression is that the Win32 library info needs an update, but, not working on targeting Windows, I'm unaware of how this would change the minimum requirements of LLVM in this OS. At https://is.gd/hTHTvX, it states that at least VC2015 is required to build LLVM. In my opinion, retargeting the Windows support to this version would be sensible, at least from what I could glean about it, albeit only on x86. |
|
Reading the documentation in VS a bit more carefully, it seems that it still supports only C89. exp2() was not defined by C89, so LLVM uses this information describing run time library support that it assumes for Windows. However, reading the info on exp2() at https://is.gd/xFJPQw, it seems that VS supports a variation of C89. Specifically, just the double precision exp2(). I'll try to inventory the variations in the math library later. |
|
Patch at https://reviews.llvm.org/D57625. |
|
If this was a regression, have we identified the commit that caused this? I ask because I want to try to understand if this change was intended behavior. |
r341095, primarily about expanding the optimizations around pow, additionally fixed a bug where exp2 would be emitted even when the TargetLibraryInfo didn't support it. Turns out Windows builds were inadvertently relying on that behavior, because that target (wrongly) thinks exp2 isn't supported. |
|
David, Could you please provide the output when running either clang-cl command above with the option --verbose? Thank you. |
|
Do you mean |
|
OK, could you please build an executable and provide a verbose output showing both how the compiler is invoked and all the libraries linked in? |
|
It would save us some round trips if you'd spell out precisely what command you'd like me to run, otherwise I am likely to misunderstand your request. |
|
Sorry, I'm not a Windows user. Based on the clang documentation, here's what I think the commands should be. Please, try: clang-cl -m32 -O2 -c exp2.cpp -o exp2.o -v And then: clang-cl -m32 exp2.o -o exp2.exe -v Thank you. |
|
E:\repro\exp2>type exp2.cpp int main(int argc, char** argv) { E:\repro\exp2>d:\clang8rc1\bin\clang-cl -m32 -O2 -c exp2.cpp -o exp2.o -v E:\repro\exp2>d:\clang8rc1\bin\clang-cl -m32 exp2.o -o exp2.exe -v |
|
Checked https://reviews.llvm.org/D57625 in. |
Great, thank you! Just to double check, are you still thinking that this should not be backported to 8.0? |
|
I'll try, after broad testing happens. |
|
LLVM described the Windows runtime library incorrectly, which D49273 exposed. The runtime support, at least for math functions, was updated in D57625 and rL353758. |
|
I'm okay with merging r353733, but let's allow it to "bake" in trunk a little longer first. |
|
I agree. r353733 and r353758 have been in the oven for nearly 24h now. Methinks that in 12h or so they'll be done out of the baking and ready to serve. Yes? Thank you. |
|
It turns out there was a lot of refactoring to these files lately. Merged r352707, r352714, r352886, r352892, r352895, r352908, r352917, r352935, r353213, r353733, and r353758 to release_80 in r353934. |
Thank you. |
Extended Description
We noticed this on 32-bit clang-cl builds.
#include
float foo(double d) { return pow(2., d / 1200.); }
At first glance I'm suspecting a bug in 2123ea7.
The text was updated successfully, but these errors were encountered: