Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rev for Cholesky on GPU #1118
Rev for Cholesky on GPU #1118
Changes from 4 commits
15be4dd
0196022
ff32352
463fb32
4f156f4
37d5b18
0d88984
ac03052
a77afda
66799ba
b87dffa
37075df
2718f05
af7f16a
69e7d9c
5f0cc21
1e27c13
6105b19
0363c08
c56c5d7
cd65c9b
eb33397
b95beba
0bf89b4
a3c1650
a82d581
96b2a40
f2bb7c4
8aa02ee
8aa1c10
c02c813
4aa3029
9df78a4
316ba55
d8b476a
decbb72
705ef7e
9846f74
59ffbf9
7197dba
32560f5
c0aeaf6
bd7b13b
671901b
2514def
e8a4e48
59aab65
483ce25
95adc28
b92441a
bef42c7
0e2ef93
1e09d95
9037b8d
5c2e48d
c45bac0
5117236
1d17106
d4c8646
d6d7fa0
d0c124f
9784cc2
0db6764
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the naming rules from Google's style guide: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules and https://google.github.io/styleguide/cppguide.html#Variable_Names
In particular, we don't use mixed case for variable names, preferring all lower case with underscores between words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variRefA_ and variRefL_ were replaced. There are similar cases in some other files, should I make a "cleanup mixed case" issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that'd be great, thanks! In retrospect I should have mentioned this earlier probably, but I felt like we already had a lot to deal with that was more important. Now things are all really good and so we're getting into marginal returns 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we switch entirely to "opencl" from "gpu?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes totally. After this PR I'm going to also make another PR changing all the
matrix_gpu
s tomatrix_cl
and rename the folderThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More on the debate on checks. For check_square I think leaving this here is fine, since performance wise this does the same thing (access to the the rows and cols members).
check_symmetric would be more performant if we did it with OpenCL, but that means that we would call check_symmetric at line 390 and before 393 but on L_A not A, so an unnecessary value_of_rec call happens if its actually not symmetric.
Perforamance wise this is nothing to worry about so I am fine leaving this as is for the sake of cleaner code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the call to
value_of_rec
is also v expensive. I'll look at this later to see if there is a more clever way to do it but I'm fine with the CPU versionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a good example of where Seans previous PR that saves flags for when these checks pass on the data would solve this without having to do any gpu stuff