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.

Sign up for GitHub

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

Jump to bottom

830 compilation with clang #831

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

830 compilation with clang #831

wants to merge 1 commit into from

Conversation

corepointer
Copy link
Collaborator

@corepointer corepointer commented Sep 24, 2024 •
edited
Loading

The commits contained in this PR fix compilation with Clang (tested version 18) and also the LLVM runtime issues with anonymous name spaces.

I tried to form the commits per problem, although they are related to the same issue, so squashing them might be inappropriate imho 🤔

ToDo:

  • Fix some test cases suffering from the namespace issue
  • Check CUDA/MPI/HDFS support

@corepointer corepointer added this to the v0.4 milestone Sep 24, 2024
@philipportner
Copy link
Collaborator

I think these are unwanted whitespace changes in RewriteToCallKernelOpPass.cpp? I'm not sure which editor you use but the .clang-format file should handle this. 1193 changes compared to 7 when hiding whitespace.

@philipportner
Copy link
Collaborator

IMO the changes should be squashed into a single commit. I think the C++20 commit should be a separate commit/PR outside of this PR and merged before if necessary for clang.

@corepointer
Copy link
Collaborator Author

I think these are unwanted whitespace changes in RewriteToCallKernelOpPass.cpp? I'm not sure which editor you use but the .clang-format file should handle this. 1193 changes compared to 7 when hiding whitespace.

I noticed the file with the big formatting changes and thought that was due to clang-format kicking in. But I didn't check tbh. I'm using CLion.

@corepointer
Copy link
Collaborator Author

IMO the changes should be squashed into a single commit. I think the C++20 commit should be a separate commit/PR outside of this PR and merged before if necessary for clang.

The commits are on the same topic but tackle distinct problems. As I read frequently in reviews to separate this and that into separate commits I thought this is the right thing to do here.

@philipportner
Copy link
Collaborator

I noticed the file with the big formatting changes and thought that was due to clang-format kicking in. But I didn't check tbh. I'm using CLion.

Maybe it's not been automatically enabled by CLion?

@corepointer
Copy link
Collaborator Author

The C++20 change is in a separate commit for the same reason. Can be cherry picked to main before handling the PR if you want ;-)

@corepointer
Copy link
Collaborator Author

I noticed the file with the big formatting changes and thought that was due to clang-format kicking in. But I didn't check tbh. I'm using CLion.

Maybe it's not been automatically enabled by CLion?

That's what I thought would happen if the .clang-format file is there 🤔

@philipportner
Copy link
Collaborator

FYI: I've formatted the code with clang-format version 18.1.8 (++20240731025011+3b5b5c1ec4a3-1~exp1~20240731145104.143).

@corepointer corepointer linked an issue Oct 4, 2024 that may be closed by this pull request
Compilation with Clang #830
Open
corepointer added a commit to corepointer/daphne that referenced this pull request Oct 6, 2024
@corepointer
[DAPHNE-daphne-eu#830] Support compilation with Clang
f2f983c 
This commit enables compiling DAPHNE with Clang (tested version 18.1.3 from Ubuntu 24 LTS). Below is the collected change log of the pre-squash commits:

[DAPHNE-daphne-eu#830] Fix duplicate symbol linker issue

Compiling with Clang/LLD complains about duplicate symbols of supportsUnaryOp and supportsBinaryOp.
The definition of supportsBinaryOp would need to be declared static if left i the BinaryOpCode.h header which is included in several places.
Since this definition is not allowed and there is only one use, it is moved to its own file.

[DAPHNE-daphne-eu#830] Replace lambda function using invalid C++ with local static template function

std::pair<bool, auto> was used as a convenience function in DaphneDSLVisitor.cpp. According to [1] this is invalid and only works with GCC. Replacing it with a templated local function makes Clang compile it too. Further improvements to this file will be considered in future commit.

[1] https://stackoverflow.com/questions/59578575/stdpairauto-auto-return-type

[DAPHNE-daphne-eu#830] Fix anonymous namespace issue when compiling with Clang

When compiling with clang, there is a issue at runtime (LLVM complaining about anonymous namespaces not allowed in various places). These are hereby replaced with a non-anonymous namespace. For the lack of creativity these are all called "file_local".

[DAPHNE-daphne-eu#830] Fix compilation error (with clang) in json dependency

The use of EOF in our json dependency makes compilation fail with Clang.
Patch is based on nlohmannjson version 3.11.3

[DAPHNE-daphne-eu#830] Silence warning when using LLD

With this new CMake configuration, we silence a lot of warnings when using the LLD linker in combination with clang:
``clang++: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]``

A CMake version bump to 3.29 is necessary for this config option. Ubuntu 24 users need to switch to the snap version of CMake. The Docker images already provide CMake 3.30

[DAPHNE-daphne-eu#830] Clang option for build.sh

This commit adds the convenience option to build with Clang to the build.sh script

Resolves daphne-eu#830, Closes daphne-eu#831
@corepointer corepointer marked this pull request as ready for review October 6, 2024 10:39
@corepointer
[DAPHNE-daphne-eu#830] Support compilation with Clang
5a16ecc 
This commit enables compiling DAPHNE with Clang (tested version 18.1.3 from Ubuntu 24 LTS). Below is the collected change log of the pre-squash commits:

[DAPHNE-daphne-eu#830] Fix duplicate symbol linker issue

Compiling with Clang/LLD complains about duplicate symbols of supportsUnaryOp and supportsBinaryOp.
The definition of supportsBinaryOp would need to be declared static if left i the BinaryOpCode.h header which is included in several places.
Since this definition is not allowed and there is only one use, it is moved to its own file.

[DAPHNE-daphne-eu#830] Replace lambda function using invalid C++ with local static template function

std::pair<bool, auto> was used as a convenience function in DaphneDSLVisitor.cpp. According to [1] this is invalid and only works with GCC. Replacing it with a templated local function makes Clang compile it too. Further improvements to this file will be considered in future commit.

[1] https://stackoverflow.com/questions/59578575/stdpairauto-auto-return-type

[DAPHNE-daphne-eu#830] Fix anonymous namespace issue when compiling with Clang

When compiling with clang, there is a issue at runtime (LLVM complaining about anonymous namespaces not allowed in various places). These are hereby replaced with a non-anonymous namespace. For the lack of creativity these are all called "file_local".

[DAPHNE-daphne-eu#830] Fix compilation error (with clang) in json dependency

The use of EOF in our json dependency makes compilation fail with Clang.
Patch is based on nlohmannjson version 3.11.3

[DAPHNE-daphne-eu#830] Silence warning when using LLD

With this new CMake configuration, we silence a lot of warnings when using the LLD linker in combination with clang:
``clang++: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]``

A CMake version bump to 3.29 is necessary for this config option. Ubuntu 24 users need to switch to the snap version of CMake. The Docker images already provide CMake 3.30

[DAPHNE-daphne-eu#830] Clang option for build.sh

This commit adds the convenience option to build with Clang to the build.sh script

Resolves daphne-eu#830, Closes daphne-eu#831
@philipportner
Copy link
Collaborator

Thanks for this patch @corepointer .

Few questions:

  1. Which version of clang did you use to test this?
  2. Did you get any warnings?
  3. Should I be able to switch between specifying --clang and omitting it without doing anything?
  4. If no to 3, what would I need to do to switch between clang and gcc? I would assume a rm -rf build should be enough?

@corepointer
Copy link
Collaborator Author

Thanks for this patch @corepointer .

👍

Few questions:

1. Which version of `clang` did you use to test this?

Version 18.1.3 - mentioned in the commit message ☝️

2. Did you get any warnings?

Tons of warnings. Besides numerous from our code(which I started to fix here and there in minor commits), there are quite many originating from our LLVM/MLIR snapshot and from the JSON library we use.

3. Should I be able to switch between specifying `--clang` and omitting it without doing anything?

4. If no to 3, what would I need to do to switch between `clang` and `gcc`? I would assume a `rm -rf build` should be enough?

No, you need to remove the current build files (either rm -rf build or ./bulid.sh --clean -y)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipportner philipportner Awaiting requested review from philipportner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

Compilation with Clang
2 participants
@corepointer @philipportner