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 CUDA demos #1139

Merged
merged 18 commits into from
Nov 22, 2024
Merged

Add CUDA demos #1139

merged 18 commits into from
Nov 22, 2024

Conversation

kchristin22
Copy link
Collaborator

This PR adds the following demos:

  • custom TensorContraction demo
  • NVIDIA's Black-Scholes sample

@kchristin22 kchristin22 self-assigned this Nov 14, 2024
Copy link
Contributor

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

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.56%. Comparing base (b5e0d12) to head (b8d7c1c).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1139   +/-   ##
=======================================
  Coverage   94.56%   94.56%           
=======================================
  Files          51       51           
  Lines        8941     8941           
=======================================
  Hits         8455     8455           
  Misses        486      486           
---- 🚨 Try these New Features:

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! 👍"


/*
* DISCLAIMER: The following file has been modified slightly to make it
* compatible with Clad. The original file can be found at NVIDIA's cuda-samples
Copy link
Owner

Choose a reason for hiding this comment

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

Can we describe what the modifications are on a high-level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I can mention that these changes refer to the original main function whose parts have been moved to a separate function to use clad::gradient on. There's also the loss of the original print statements and the addition of helper functions to verify the results. Would this do as a whole?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about now?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, or just make a diff and put it in comment if it’s not big.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that would be confusing, because I kind of restructured the file

demos/CUDA/TensorContraction.cu Outdated Show resolved Hide resolved
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

@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

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
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

@@ -1893,6 +1893,10 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context,
const Expr* arg = CE->getArg(i);
const auto* PVD = FD->getParamDecl(
i - static_cast<unsigned long>(isMethodOperatorCall));
if (PVD->getType()->isRValueReferenceType()) {
IdentifierInfo* RValueName = CreateUniqueIdentifier("_r");
const_cast<ParmVarDecl*>(PVD)->setDeclName(RValueName);
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]

        const_cast<ParmVarDecl*>(PVD)->setDeclName(RValueName);
        ^

@kchristin22
Copy link
Collaborator Author

I added as many parts as possible from the original demo to minimize the diff. You're free to have a look in case you have any suggestions!

Copy link
Contributor

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

1 similar comment
Copy link
Contributor

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

@kchristin22
Copy link
Collaborator Author

@vgvassilev Let's postpone this merge till the r value PR is fixed and merged, as this functionality is needed to run the demo

Copy link
Contributor

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

2 similar comments
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
Owner

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

@vgvassilev vgvassilev merged commit 2f268db into vgvassilev:master Nov 22, 2024
89 of 90 checks passed
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