Enable exp10 libcall on linux #68736
Conversation
|
@RKSimon should exp10/exp10f be available for -ps4/-ps5 triples? |
No |
Krishna-13-cyber
force-pushed
the
exp10-linux
branch
from
7a064ce
to
08f3669
Compare
| @@ -1,3 +1,4 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3 | |||
There was a problem hiding this comment.
Switch to generated checks should be committed separately
Move to using update_test_checks script + cleanup check-prefixes. Help simplify diff for #68736
Krishna-13-cyber
force-pushed
the
exp10-linux
branch
from
08f3669
to
673d2eb
Compare
| @@ -139,9 +139,10 @@ define fp128 @powl_exp2l_not_fast(fp128 %x, fp128 %y) { | |||
| ; TODO: exp10() is not widely enabled by many targets yet. | |||
Move to using update_test_checks script + cleanup check-prefixes. Help simplify diff for llvm#68736
| // exp10, exp10f, exp10l is available on Linux (GLIBC) but are extremely | ||
| // buggy prior to glibc version 2.18. Until this version is widely deployed | ||
| // or we have a reasonable detection strategy, we cannot use exp10 reliably | ||
| // on Linux. | ||
| // |
There was a problem hiding this comment.
I would leave some kind of historical note
There was a problem hiding this comment.
Oh okay, thanks again!
There was a problem hiding this comment.
Just update the "Until this version is widely deployed or we have a reasonable detection strategy, we cannot use exp10 reliably" part to something about how this is so old it doesn't matter
There was a problem hiding this comment.
"As this version is so old, we don't really need to worry about using exp10 until we find more good ways to detect it."
Does this look fine?
There was a problem hiding this comment.
The "until we find more good ways to detect it." reads off. Can drop it or rephrase
There was a problem hiding this comment.
Yes, I will drop this.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Bot failure is real |
| // | ||
| // Fall through to disable all of them. | ||
| [[fallthrough]]; | ||
| default: |
There was a problem hiding this comment.
This change here caused a quite noisy warning from GCC:
../lib/Analysis/TargetLibraryInfo.cpp: In function ‘void initialize(llvm::TargetLibraryInfoImpl&, const llvm::Triple&, llvm::ArrayRef<llvm::StringLiteral>)’:
../lib/Analysis/TargetLibraryInfo.cpp:533:10: warning: enumeration value ‘UnknownOS’ not handled in switch [-Wswitch]
533 | switch (T.getOS()) {
| ^
../lib/Analysis/TargetLibraryInfo.cpp:533:10: warning: enumeration value ‘Darwin’ not handled in switch [-Wswitch]
../lib/Analysis/TargetLibraryInfo.cpp:533:10: warning: enumeration value ‘DragonFly’ not handled in switch [-Wswitch]
../lib/Analysis/TargetLibraryInfo.cpp:533:10: warning: enumeration value ‘FreeBSD’ not handled in switch [-Wswitch]
../lib/Analysis/TargetLibraryInfo.cpp:533:10: warning: enumeration value ‘Fuchsia’ not handled in switch [-Wswitch]
../lib/Analysis/TargetLibraryInfo.cpp:533:10: warning: enumeration value ‘KFreeBSD’ not handled in switch [-Wswitch]
(and so on for all OSes known by Triple)
So ideally we'd keep the default case to silence this compiler warning (or not enable -Wswitch, wherever that comes from).
|
Should this patch has nothing to do with PS? I tried adding |
This reverts commit 9848fa4. Causes buildbot failures.
|
I went ahead and reverted this change. I could have fixed the uncovered switch warning, but the change in general looks problematic to me. The comment indicates that these functions are broken prior to glibc 2.18 -- however, a common glibc baseline for binary distribution is glibc 2.17, because it is used by CentOS/RHEL 7. This is the glibc baseline that Rust uses, for example. If you want to increase the glibc requirements of LLVM-generated binaries, then that needs a wider discussion than this pull request (or more research to show that this is safe, e.g. because distros using glibc 2.17 have backported the necessary fixes or so). There are also some buildbots failing with |
This reverts commit 9848fa4. Causes buildbot failures.
No description provided.