Skip to content
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

Changes to compile with Clang17 #10363

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

czentgr
Copy link
Collaborator

@czentgr czentgr commented Jul 1, 2024

The changes fix:

  1. cast issue with the integer max to a floating point
    implicit conversion from 'long' to 'double' changes value from
       9223372036854775807 to 9223372036854775808
    implicit conversion from '__int128' to 'float' changes value from
       170141183460469231731687303715884105727
       to 1.70141183460469231731687E+38
  1. template instantiation of simple vectors with complex types in tests
/root/oss/velox/./velox/vector/SimpleVector.h:370:17: error: invalid operands to binary expression ('const facebook::velox::Array<int>' and 'const facebook::velox::Array<int>')
  370 |     return left < right ? -1 : left == right ? 0 : 1;
      |            ~~~~ ^ ~~~~~
/root/oss/velox/./velox/vector/SimpleVector.h:163:19: note: in instantiation of member function 'facebook::velox::SimpleVector<facebook::velox::Array<int>>::comparePrimitiveAsc' requested here
  163 |     auto result = comparePrimitiveAsc(thisValue, otherValue);
      |        
  1. Clang needs lib atomic symbols to be resolved for a number of executables.
/opt/rh/gcc-toolset-13/root/usr/lib/gcc/x86_64-redhat-linux/13/../../../../bin/ld: velox/exec/libvelox_exec.a(Task.cpp.o): in function `std::__atomic_base<unsigned int>::is_lock_free() const':
/opt/rh/gcc-toolset-13/root/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/atomic_base.h:460: undefined reference to `__atomic_is_lock_free'
/opt/rh/gcc-toolset-13/root/usr/lib/gcc/x86_64-redhat-linux/13/../../../../bin/ld: velox/exec/libvelox_exec.a(Task.cpp.o): in function `std::__atomic_base<long>::is_lock_free() const':
/opt/rh/gcc-toolset-13/root/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/atomic_base.h:460: undefined reference to `__atomic_is_lock_free'clang++: error: linker command failed with exit code 1 (use -v to see invocation)ninja: build stopped: subcommand failed.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 1, 2024
@czentgr
Copy link
Collaborator Author

czentgr commented Jul 1, 2024

@majetideepak FYI

Copy link

netlify bot commented Jul 1, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit b30bcf8
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/668dbff5d838790008976e6d

Makefile Outdated Show resolved Hide resolved
@czentgr czentgr force-pushed the cz_prepare_clang17 branch 3 times, most recently from e3908cf to 6ccb99e Compare July 2, 2024 18:09
@@ -4401,13 +4407,18 @@ struct DictColumnWriterTestCase {
context.initBuffer();

// complexVectorType will be nullptr if the vector is not complex.
/*
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove old code.

@czentgr
Copy link
Collaborator Author

czentgr commented Jul 2, 2024

Need to work on this a bit more. Some of the deps built don't pick up the change. For example arrow:

16:48:46  #12 35.16 [203/860] Performing configure step for 'arrow_ep'
16:48:46  #12 35.16 -- Building using CMake version: 3.28.3
16:48:46  #12 35.16 -- The C compiler identification is GNU 12.2.1
16:48:46  #12 35.16 -- The CXX compiler identification is GNU 12.2.1

I guess the variables do not get inherited into bundled build with their own projects. Probably needs a few more changes.

Dependencies (from the setup scripts are also built with gcc).

@czentgr czentgr marked this pull request as ready for review July 3, 2024 14:53
@czentgr czentgr changed the title [WIP] Changes to compile with Clang17 Changes to compile with Clang17 Jul 3, 2024
velox/type/DecimalUtil.h Outdated Show resolved Hide resolved
velox/type/DecimalUtil.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @czentgr . Changes look good to me.
We should add a CI test as well to ensure support is not broken.
@kgpai any thoughts on this?

@majetideepak
Copy link
Collaborator

I think we have to support building deps with clang17 as well.

@czentgr
Copy link
Collaborator Author

czentgr commented Jul 9, 2024

So recently Centos9 updated their clang from version 17 to 18 and it causes additional build breaks (around arrays that use a variable input size and possibly more). As a result I've installed clang15 (the only other easily available version from the repo) and built with that.
As a result of using clang15 I also found out how to make it available in ccache (not done by default after installing the package). Cmake doesn't seem to have a good way of changing the default compiler especially in external projects. Cmake picks up the CC and CXX environment variables - and this would apply to subprojects made when building.

Overall improvement of 99 TPCDS queries is about 8% compared to gcc12. Some queries are slightly slower and some are faster. Spread is 0.65 to 3.4 ratio. That is the worst query took almost twice as long (this one is a subsecond query) and best query improved by factor 3ish (19s down to 5.5s) .

@czentgr czentgr force-pushed the cz_prepare_clang17 branch 3 times, most recently from 651e30d to 0d6d025 Compare July 9, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants