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 osx ci #1025

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add llvm 18 osx ci #1025

wants to merge 7 commits into from

Conversation

vgvassilev
Copy link
Owner

This is the same as #889 but I cannot login to the bots. Let's try if this will let me use tmate...

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

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

// Silence diag outputs in nested derivation process.
pullbackRequest.VerboseDiags = false;
// Silence diag outputs in nested derivation process unless we are going
// through a builtin function which we know we cannot differentiate.
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: argument comment missing for literal argument 'val' [bugprone-argument-comment]

Suggested change
// through a builtin function which we know we cannot differentiate.
DerivedCallArgs.front()->getType(), m_Context, /*val=*/1));

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

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.25%. Comparing base (3ca7de7) to head (fe4d8a0).
Report is 2 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1025      +/-   ##
==========================================
+ Coverage   94.24%   94.25%   +0.01%     
==========================================
  Files          55       55              
  Lines        8445     8445              
==========================================
+ Hits         7959     7960       +1     
+ Misses        486      485       -1     
Files with missing lines Coverage Δ
include/clad/Differentiator/BuiltinDerivatives.h 100.00% <ø> (ø)

... and 2 files with indirect coverage changes

Files with missing lines Coverage Δ
include/clad/Differentiator/BuiltinDerivatives.h 100.00% <ø> (ø)

... and 2 files with indirect coverage changes

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

There were too many comments to post at once. Showing the first 10 out of 11. Check the log or trigger a new build to see more.

@@ -43,7 +43,7 @@ struct DiffRequest {
unsigned RequestedDerivativeOrder = 1;
/// Context in which the function is being called, or a call to
/// clad::gradient/differentiate, where function is the first arg.
clang::CallExpr* CallContext = nullptr;
const clang::CallExpr* CallContext = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: member variable 'CallContext' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

  const clang::CallExpr* CallContext = nullptr;
                         ^

"have a "
"definition",
{FD->getNameAsString()});
if (true || request.VerboseDiags) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]

Suggested change
if (true || request.VerboseDiags) {
if (true) {

@@ -176,7 +176,7 @@ namespace clad {

void DiffRequest::updateCall(FunctionDecl* FD, FunctionDecl* OverloadedFD,
Sema& SemaRef) {
CallExpr* call = this->CallContext;
CallExpr* call = const_cast<CallExpr*>(this->CallContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

    CallExpr* call = const_cast<CallExpr*>(this->CallContext);
                     ^

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

@@ -176,7 +176,7 @@ namespace clad {

void DiffRequest::updateCall(FunctionDecl* FD, FunctionDecl* OverloadedFD,
Sema& SemaRef) {
CallExpr* call = this->CallContext;
CallExpr* call = const_cast<CallExpr*>(this->CallContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]

Suggested change
CallExpr* call = const_cast<CallExpr*>(this->CallContext);
auto* call = const_cast<CallExpr*>(this->CallContext);

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

@@ -77,6 +77,9 @@ ValueAndPushforward<int, int> cudaDeviceSynchronize_pushforward()
}
#endif

// NOLINTBEGIN(readability-identifier-naming)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' comment [clang-tidy-nolint]

// NOLINTBEGIN(readability-identifier-naming)
   ^

// FIXME: Add the rest of the __builtin_ routines for log, sqrt, abs, etc.

// NOLINTEND(readability-identifier-naming)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' comment [clang-tidy-nolint]

// NOLINTEND(readability-identifier-naming)
   ^

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

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


CUDA_HOST_DEVICE inline ValueAndPushforward<float, float>
__builtin_sqrtf_pushforward(float x, float d_x) {
return {builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * 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: use of undeclared identifier 'T' [clang-diagnostic-error]

  return {builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * d_x};
                                         ^


CUDA_HOST_DEVICE inline ValueAndPushforward<float, float>
__builtin_sqrtf_pushforward(float x, float d_x) {
return {builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * 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: use of undeclared identifier 'T' [clang-diagnostic-error]

  return {builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * d_x};
                               ^


CUDA_HOST_DEVICE inline ValueAndPushforward<float, float>
__builtin_sqrtf_pushforward(float x, float d_x) {
return {builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * 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: use of undeclared identifier 'builtin_sqrtf'; did you mean '__builtin_sqrtf'? [clang-diagnostic-error]

Suggested change
return {builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * d_x};
return {__builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * d_x};
Additional context

/usr/include/c++/12/cmath:463: '__builtin_sqrtf' declared here

  { return __builtin_sqrtf(__x); }
           ^


CUDA_HOST_DEVICE inline ValueAndPushforward<float, float>
__builtin_sqrtf_pushforward(float x, float d_x) {
return {builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * 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: use of undeclared identifier 'builtin_sqrtf'; did you mean '__builtin_sqrtf'? [clang-diagnostic-error]

Suggested change
return {builtin_sqrtf(x), (((T)1) / (((T)2) * builtin_sqrtf(x))) * d_x};
return {builtin_sqrtf(x), (((T)1) / (((T)2) * __builtin_sqrtf(x))) * d_x};
Additional context

/usr/include/c++/12/cmath:463: '__builtin_sqrtf' declared here

  { return __builtin_sqrtf(__x); }
           ^


CUDA_HOST_DEVICE inline ValueAndPushforward<double, double>
__builtin_sqrt_pushforward(double x, double d_x) {
return {builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * 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: use of undeclared identifier 'T' [clang-diagnostic-error]

  return {builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * d_x};
                                        ^


CUDA_HOST_DEVICE inline ValueAndPushforward<double, double>
__builtin_sqrt_pushforward(double x, double d_x) {
return {builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * 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: use of undeclared identifier 'T' [clang-diagnostic-error]

  return {builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * d_x};
                              ^


CUDA_HOST_DEVICE inline ValueAndPushforward<double, double>
__builtin_sqrt_pushforward(double x, double d_x) {
return {builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * 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: use of undeclared identifier 'builtin_sqrt'; did you mean '__builtin_sqrt'? [clang-diagnostic-error]

Suggested change
return {builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * d_x};
return {__builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * d_x};
Additional context

/usr/include/c++/12/cmath:475: '__builtin_sqrt' declared here

    { return __builtin_sqrt(__x); }
             ^


CUDA_HOST_DEVICE inline ValueAndPushforward<double, double>
__builtin_sqrt_pushforward(double x, double d_x) {
return {builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * 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: use of undeclared identifier 'builtin_sqrt'; did you mean '__builtin_sqrt'? [clang-diagnostic-error]

Suggested change
return {builtin_sqrt(x), (((T)1) / (((T)2) * builtin_sqrt(x))) * d_x};
return {builtin_sqrt(x), (((T)1) / (((T)2) * __builtin_sqrt(x))) * d_x};
Additional context

/usr/include/c++/12/cmath:475: '__builtin_sqrt' declared here

    { return __builtin_sqrt(__x); }
             ^

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@mcbarton
Copy link
Contributor

mcbarton commented Oct 15, 2024

@vgvassilev If we can work out how to get this PR passing then I'll look at adding llvm 19 compatibility.

@vgvassilev
Copy link
Owner Author

@vgvassilev If we can work out how to get this PR passing then I'll look at adding llvm 19 compatibility.

On my todo list... I have everything set up, don't have the time to look at it just yet.

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