Resolve Clang issues in DataFormats/Math #7223
Conversation
Clang `vector_size` doesn't allow value dependent expressions in templates. Instead use `ext_vector_type` (OpenCL vector types), which doesn't have this limitation. See PR16986: http://llvm.org/bugs/show_bug.cgi?id=16986 Previous version of this patch had an issue where `Vec4<float>` and `Vec2<double>` were lowered to the same type. From Clang AST dump: |-TypedefDecl .. referenced Vec4F 'Vec4<float>':'float __attribute__((ext_vector_type(16)))' |-TypedefDecl .. referenced Vec2D 'Vec2<double>':'float __attribute__((ext_vector_type(16)))' Updated patch resolves it. Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
It looks a bit complicated on understanding what can and cannot be
`constexpr`. For example `std::cos` (cmath) is not `constexpr`, but in GCC
is can be lowered down to `__builtin_cos`, which will be `constexpr`.
Clang is more strict and does not treat `__builtin_*` as `constexpr`.
With GCC `std::cos` with `-fno-builtin` will also yield a compiler
error.
This could cause issues in C++14 (?)
Commit 724b4eb62820414bc79c35c90825481d7c0f3c59 from C++ Standard Draft.
An implementation shall not declare any standard library function
signature as \tcode{constexpr} except for those where it is
explicitly required.
I found the fellowing to cover the issue quite well:
http://stackoverflow.com/questions/27744079/is-it-a-conforming-compiler-extension-to-treat-non-constexpr-standard-library-fu
Expect more issues with this file as GCC might revert the changes and
built-ins will not be `constexpr`:
https://gcc.gnu.org/ml/libstdc++/2014-09/msg00004.html
Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
- `eta3` removed as not used; - `std::abs` from C++11 needs to be used, which supports `long long`, otherwise results might be truncated to `int`. Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
|
A new Pull Request was created by @davidlt for CMSSW_7_4_X. Resolve Clang issues in DataFormats/Math It involves the following packages: DataFormats/Math @cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks. |
|
More details in commit messages as usual. |
| typedef double __attribute__( ( vector_size( 16 ) ) ) float64x2_t; | ||
| typedef double __attribute__( ( vector_size( 32 ) ) ) float64x4_t; | ||
| typedef double __attribute__( ( vector_size( 64 ) ) ) float64x8_t; | ||
| #ifdef __clang__ |
There was a problem hiding this comment.
how did we get to having to do fixes here.
I recall we switched to SSE in #6611
So, how did clang see an issue here?
There was a problem hiding this comment.
Because we have DataFormats/Math/src/ExtVec.cc, which is always compiled. That's the one trigger compilation issues. It contains a number of operators for printing things like Vec2D. One could disable those, but there is no top level header file which selects USE_EXTVECT/USE_SSEVECT. Current decision is done in DataFormats/Math/interface/SIMDVec.h, but you cannot include it into DataFormats/Math/src/ExtVec.cc as that would create cyclic dependency. Someone would need to split SIMDVec.h.
There was a problem hiding this comment.
ExtVec.cc, makes sense now.
Thanks.
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request will be automatically merged. |
Resolve Clang issues in DataFormats/Math
This patchset resolves build errors and warnings with Clang compiler. Most of this was already available in CLANG IB (kudos to Giulio), but there was an issue where
Vec4FandVec2Dwere lowered to the same type. Most of this can be pulled out once Clang improvesvector_sizeimplementation.The biggest issue is
constexprin the test. The test was disabled on Clang, but most likely will fail with GCC in future. C++ standard was updated and freedom to addconstexprto random functions in standard library were removed. Thusconstexpr double a = std::cos()is not a valid code.http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3788.html#2013
Build tested with GCC 4.9.1 and Clang pre-3.6 (65d8b4c) in DEVEL IB.