-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc] Make LIBC_MATH_NO_ERRNO imply -fno-math-errno
#125794
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
Conversation
This partially reverts llvm#124200. Rather than using a CMake option to control whether to enable `-fno-math-errno`, use LIBC_MATH_NO_ERRNO configuration option. While there might be other cases when we want to set `-fno-math-errno`, having LIBC_MATH_NO_ERRNO imply it should be always safe and represents a reasonable starting point.
|
@llvm/pr-subscribers-libc Author: Petr Hosek (petrhosek) ChangesThis partially reverts #124200. Rather than using a CMake option to control whether to enable Full diff: https://github.com/llvm/llvm-project/pull/125794.diff 3 Files Affected:
diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 0facb0b9be0c13..f5f638bef5c1d8 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -28,13 +28,6 @@ function(_get_compile_options_from_flags output_var)
elseif(LIBC_TARGET_ARCHITECTURE_IS_RISCV64)
list(APPEND compile_options "-D__LIBC_RISCV_USE_FMA")
endif()
- # For clang, we will build the math functions with `-fno-math-errno` so that
- # __builtin_fma* will generate the fused-mutliply-add instructions. We
- # don't put the control flag to the public config yet, and see if it makes
- # sense to just enable this flag by default.
- if(LIBC_ADD_FNO_MATH_ERRNO)
- list(APPEND compile_options "-fno-math-errno")
- endif()
endif()
if(ADD_ROUND_OPT_FLAG)
if(LIBC_TARGET_ARCHITECTURE_IS_X86_64)
@@ -71,6 +64,9 @@ function(_get_compile_options_from_flags output_var)
"SHELL:-Xclang -target-feature -Xclang +fullfp16")
endif()
endif()
+ if(LIBC_CONF_MATH_OPTIMIZATIONS MATCHES "LIBC_MATH_NO_ERRNO")
+ list(APPEND compile_options "-fno-math-errno")
+ endif()
elseif(MSVC)
if(ADD_FMA_FLAG)
list(APPEND compile_options "/arch:AVX2")
diff --git a/libc/cmake/modules/LLVMLibCTestRules.cmake b/libc/cmake/modules/LLVMLibCTestRules.cmake
index 10bb9c9487d636..96fa6c3a707e47 100644
--- a/libc/cmake/modules/LLVMLibCTestRules.cmake
+++ b/libc/cmake/modules/LLVMLibCTestRules.cmake
@@ -1,11 +1,6 @@
function(_get_common_test_compile_options output_var c_test flags)
_get_compile_options_from_flags(compile_flags ${flags})
- # Remove -fno-math-errno if it was added.
- if(LIBC_ADD_FNO_MATH_ERRNO)
- list(REMOVE_ITEM compile_options "-fno-math-errno")
- endif()
-
set(compile_options
${LIBC_COMPILE_OPTIONS_DEFAULT}
${LIBC_TEST_COMPILE_OPTIONS_DEFAULT}
diff --git a/libc/config/baremetal/config.json b/libc/config/baremetal/config.json
index 08c581d1c68226..d29fb64e370ddb 100644
--- a/libc/config/baremetal/config.json
+++ b/libc/config/baremetal/config.json
@@ -28,7 +28,7 @@
},
"math": {
"LIBC_CONF_MATH_OPTIMIZATIONS": {
- "value": "(LIBC_MATH_SKIP_ACCURATE_PASS | LIBC_MATH_SMALL_TABLES)"
+ "value": "(LIBC_MATH_SKIP_ACCURATE_PASS | LIBC_MATH_SMALL_TABLES | LIBC_MATH_NO_ERRNO)"
}
}
}
|
| "SHELL:-Xclang -target-feature -Xclang +fullfp16") | ||
| endif() | ||
| endif() | ||
| if(LIBC_CONF_MATH_OPTIMIZATIONS MATCHES "LIBC_MATH_NO_ERRNO") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to L101-ish, where LIBC_CONF_MATH_OPTIMIZATIONS is checked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| # Remove -fno-math-errno if it was added. | ||
| if(LIBC_ADD_FNO_MATH_ERRNO) | ||
| list(REMOVE_ITEM compile_options "-fno-math-errno") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there tests failed because of this part? Last time I tried, there are failed math tests that check errno when the tests are built with -fno-math-errno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any test failures on the presubmit bots, should those exercise all tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the tests failed when the flag is set. I don't think any of our bots running baremetal config, so they don't cover this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't currently run tests on baremetal so I don't know if we have any way to find out.
This partially reverts #124200. Rather than using a CMake option to control whether to enable
-fno-math-errno, use LIBC_MATH_NO_ERRNO configuration option. While there might be other cases when we want to set-fno-math-errno, having LIBC_MATH_NO_ERRNO imply it should be always safe and represents a reasonable starting point.