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

Add llvm 18 for osx to ci #889

Closed
wants to merge 4 commits into from

Conversation

mcbarton
Copy link
Contributor

@mcbarton mcbarton commented May 8, 2024

This PR adds llvm 18 to the ci for osx

Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.37%. Comparing base (d82f7fd) to head (be5679c).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #889    +/-   ##
========================================
  Coverage   94.37%   94.37%            
========================================
  Files          50       50            
  Lines        8366     8686   +320     
========================================
+ Hits         7895     8197   +302     
- Misses        471      489    +18     

see 33 files with indirect coverage changes

see 33 files with indirect coverage changes

@mcbarton mcbarton marked this pull request as draft May 8, 2024 15:16
@vgvassilev
Copy link
Owner

@mcbarton, what is the fate of this PR?

@mcbarton
Copy link
Contributor Author

mcbarton commented Jun 2, 2024

@vgvassilev I'll take another look at this PR in the coming week (got sidetracked with the work on xeus-cpp). I just need to make the ci avoid the need for curling a binary from the llvm project Github page, and it should work.

@mcbarton
Copy link
Contributor Author

mcbarton commented Jun 6, 2024

@vgvassilev are you able to take a look at this, or have someone else who can finish it off? Clad is building and the tests are running with llvm 18. I'm just a little too busy at the moment to work out why they some of them are failing and fix them (I also don't know which are the ones which are expected to fail).

@vgvassilev
Copy link
Owner

Looks like there is a single failure that might be hard to debug. Perhaps we should add a debug build based on clang-18 so that we can debug such issues on the bots.

@vgvassilev vgvassilev force-pushed the add-llvm-18-osx-ci branch 2 times, most recently from 82f91c4 to ba194a0 Compare July 29, 2024 21:01
@mcbarton
Copy link
Contributor Author

@vaithak Any ideas what the issue with this PR here, based on the output from the ci? At least one of the issues seems to have been introduced in the PR you was part of here #873

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -135,6 +135,11 @@ CUDA_HOST_DEVICE inline void __builtin_powf_pullback(float x, float exponent,
*d_exponent += t.pushforward * d_y;
}

CUDA_HOST_DEVICE ValueAndPushforward<double, double>
__builtin_exp_pushforward(double x, double d_x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: declaration uses identifier '__builtin_exp_pushforward', which is a reserved identifier [bugprone-reserved-identifier]

__builtin_exp_pushforward(double x, double d_x) {
^

this fix will not be applied because it overlaps with another fix

@@ -135,6 +135,11 @@
*d_exponent += t.pushforward * d_y;
}

CUDA_HOST_DEVICE ValueAndPushforward<double, double>
__builtin_exp_pushforward(double x, double d_x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function '__builtin_exp_pushforward' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]

__builtin_exp_pushforward(double x, double d_x) {
^
Additional context

include/clad/Differentiator/BuiltinDerivatives.h:138: make as 'inline'

__builtin_exp_pushforward(double x, double d_x) {
^

@@ -135,6 +135,11 @@
*d_exponent += t.pushforward * d_y;
}

CUDA_HOST_DEVICE ValueAndPushforward<double, double>
__builtin_exp_pushforward(double x, double d_x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for function '__builtin_exp_pushforward' [readability-identifier-naming]

__builtin_exp_pushforward(double x, double d_x) {
^

this fix will not be applied because it overlaps with another fix

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -135,6 +135,11 @@ CUDA_HOST_DEVICE inline void __builtin_powf_pullback(float x, float exponent,
*d_exponent += t.pushforward * d_y;
}

CUDA_HOST_DEVICE ValueAndPushforward<double, double>
inline __builtin_exp_pushforward(double x, double d_x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: declaration uses identifier '__builtin_exp_pushforward', which is a reserved identifier [bugprone-reserved-identifier]

inline __builtin_exp_pushforward(double x, double d_x) {
       ^

this fix will not be applied because it overlaps with another fix

@@ -135,6 +135,11 @@
*d_exponent += t.pushforward * d_y;
}

CUDA_HOST_DEVICE ValueAndPushforward<double, double>
inline __builtin_exp_pushforward(double x, double d_x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for function '__builtin_exp_pushforward' [readability-identifier-naming]

inline __builtin_exp_pushforward(double x, double d_x) {
       ^

this fix will not be applied because it overlaps with another fix

@vgvassilev vgvassilev mentioned this pull request Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants