LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 40541 - LLVM 8 stopped optimizing pow(2., x) to exp2(x)
Summary: LLVM 8 stopped optimizing pow(2., x) to exp2(x)
Status: RESOLVED FIXED
Alias: None
Product: clang
Classification: Unclassified
Component: -New Bugs (show other bugs)
Version: 8.0
Hardware: PC Windows NT
: P enhancement
Assignee: Unassigned Clang Bugs
URL:
Keywords:
Depends on:
Blocks: release-8.0.0
  Show dependency tree
 
Reported: 2019-01-30 15:33 PST by dmajor
Modified: 2019-02-13 08:25 PST (History)
11 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description dmajor 2019-01-30 15:33:12 PST
We noticed this on 32-bit clang-cl builds.

#include <cmath>
float foo(double d) { return pow(2., d / 1200.); }

> d:\clang7.0.1\bin\clang-cl -m32 -O2 -c exp2.cpp
> dumpbin /all exp2.obj | findstr REL32
 00000017  REL32                      00000000         F  _exp2

> d:\clang8rc1\bin\clang-cl -m32 -O2 -c exp2.cpp
> dumpbin /all exp2.obj | findstr REL32
 0000002C  REL32                      00000000        11  _pow

At first glance I'm suspecting a bug in https://github.com/llvm/llvm-project/commit/2123ea7d5c722e98ce0f047c8baa3fb3db23e6b3.
Comment 1 Evandro Menezes 2019-01-30 16:20:27 PST
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
Comment 2 dmajor 2019-01-31 10:28:58 PST
Is this something that could be safely fixed within the 8.0.0 release?
Comment 3 Evandro Menezes 2019-01-31 11:31:41 PST
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.
Comment 4 Evandro Menezes 2019-01-31 12:30:15 PST
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.
Comment 5 Evandro Menezes 2019-02-01 15:10:07 PST
Patch at https://reviews.llvm.org/D57625.
Comment 6 Reid Kleckner 2019-02-05 10:44:23 PST
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.
Comment 7 dmajor 2019-02-05 10:51:44 PST
(In reply to Reid Kleckner from comment #6)
> 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.
Comment 8 Evandro Menezes 2019-02-05 15:53:51 PST
David,

Could you please provide the output when running either clang-cl command above with the option --verbose?

Thank you.
Comment 9 dmajor 2019-02-05 17:30:01 PST
Do you mean `-v`? The `--verbose` option isn't recognized. What part of the output are you looking for?
Comment 10 Evandro Menezes 2019-02-05 17:38:03 PST
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?
Comment 11 dmajor 2019-02-05 17:44:01 PST
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.
Comment 12 Evandro Menezes 2019-02-05 18:07:13 PST
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.
Comment 13 dmajor 2019-02-06 14:13:36 PST
E:\repro\exp2>type exp2.cpp
#include <cmath>

int main(int argc, char** argv) {
    return pow(2., argc);
}

E:\repro\exp2>d:\clang8rc1\bin\clang-cl -m32 -O2 -c exp2.cpp -o exp2.o -v
clang version 8.0.0 (tags/RELEASE_800/rc1 352000)
Target: i386-pc-windows-msvc
Thread model: posix
InstalledDir: d:\clang8rc1\bin
 "d:\\clang8rc1\\bin\\clang-cl.exe" -cc1 -triple i386-pc-windows-msvc19.16.27026 -emit-obj -mincremental-linker-compatible -disable-free -disable-llvm-verifier -discard-value-names -main-file-name exp2.cpp -mrelocation-model static -mthread-model posix -relaxed-aliasing -fmath-errno -masm-verbose -mconstructor-aliases -target-cpu pentium4 -mllvm -x86-asm-syntax=intel -D_MT -flto-visibility-public-std --dependent-lib=libcmt --dependent-lib=oldnames -stack-protector 2 -fms-volatile -fdiagnostics-format msvc -dwarf-column-info -momit-leaf-frame-pointer -v -ffunction-sections -coverage-notes-file "E:\\repro\\exp2\\exp2.gcno" -resource-dir "d:\\clang8rc1\\lib\\clang\\8.0.0" -internal-isystem "d:\\clang8rc1\\lib\\clang\\8.0.0\\include" -internal-isystem "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Community\\VC\\Tools\\MSVC\\14.16.27023\\include" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\Include\\10.0.17134.0\\ucrt" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.17134.0\\shared" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.17134.0\\um" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.17134.0\\winrt" -O2 -fdeprecated-macro -fdebug-compilation-dir "E:\\repro\\exp2" -ferror-limit 19 -fmessage-length 120 -fno-use-cxa-atexit -fms-extensions -fms-compatibility -fms-compatibility-version=19.16.27026 -std=c++14 -fdelayed-template-parsing -fobjc-runtime=gcc -fdiagnostics-show-option -fcolor-diagnostics -vectorize-loops -vectorize-slp -o exp2.o -x c++ exp2.cpp -faddrsig
clang -cc1 version 8.0.0 based upon LLVM 8.0.0 default target x86_64-pc-windows-msvc
#include "..." search starts here:
#include <...> search starts here:
 d:\clang8rc1\lib\clang\8.0.0\include
 C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include
 C:\Program Files (x86)\Windows Kits\10\Include\10.0.17134.0\ucrt
 C:\Program Files (x86)\Windows Kits\10\include\10.0.17134.0\shared
 C:\Program Files (x86)\Windows Kits\10\include\10.0.17134.0\um
 C:\Program Files (x86)\Windows Kits\10\include\10.0.17134.0\winrt
End of search list.

E:\repro\exp2>d:\clang8rc1\bin\clang-cl -m32 exp2.o -o exp2.exe -v
clang version 8.0.0 (tags/RELEASE_800/rc1 352000)
Target: i386-pc-windows-msvc
Thread model: posix
InstalledDir: d:\clang8rc1\bin
 "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Community\\VC\\Tools\\MSVC\\14.16.27023\\bin\\HostX64\\x86\\link.exe" -out:exp2.exe "-libpath:C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Community\\VC\\Tools\\MSVC\\14.16.27023\\lib\\x86" "-libpath:C:\\Program Files (x86)\\Windows Kits\\10\\Lib\\10.0.17134.0\\ucrt\\x86" "-libpath:C:\\Program Files (x86)\\Windows Kits\\10\\Lib\\10.0.17134.0\\um\\x86" -nologo exp2.o
Comment 14 Evandro Menezes 2019-02-11 12:18:54 PST
Checked https://reviews.llvm.org/D57625 in.
Comment 15 dmajor 2019-02-11 14:41:22 PST
(In reply to Evandro Menezes from comment #14)
> 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?
Comment 16 Evandro Menezes 2019-02-11 14:49:21 PST
I'll try, after broad testing happens.
Comment 17 Evandro Menezes 2019-02-11 14:53:06 PST
LLVM described the Windows runtime library incorrectly, which D49273 exposed.  The runtime support, at least for math functions, was updated in D57625 and rL353758.
Comment 18 Hans Wennborg 2019-02-12 03:23:44 PST
I'm okay with merging r353733, but let's allow it to "bake" in trunk a little longer first.
Comment 19 Evandro Menezes 2019-02-12 09:29:13 PST
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.
Comment 20 Hans Wennborg 2019-02-13 02:31:01 PST
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.
Comment 21 Evandro Menezes 2019-02-13 08:25:41 PST
(In reply to Hans Wennborg from comment #20)
> Merged r352707, r352714, r352886, r352892, r352895, r352908, r352917,
> r352935, r353213, r353733, and r353758 to release_80 in r353934.

Thank you.