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
Fix warnings from AssertCuda and AssertCusparse #6111
Conversation
include/deal.II/base/exceptions.h
Outdated
#define AssertCuda(error_code) Assert(error_code == cudaSuccess, \ | ||
dealii::ExcCudaError(cudaGetErrorString(error_code))) | ||
#else | ||
#define AssertCuda(error_code) { (void) error_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.
Since error_code
can be a complicated expression, you probably want to write this as
(void) (error_code);
But the substitution is not the same -- if error_code
involves function calls, the old code omitted these function calls in release mode, whereas now you are actually evaluating them. I don't know whether that happens anywhere, but this is the reason why Assert
does not evaluate its operand in release mode.
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.
error_code
is always an int
, it cannot be anything else but I can change the code if you want.
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.
But it's conceivable that you call the macro as in AssertCuda(some_object.check_internal_consistency())
where the inner function call returns an int
.
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.
I can't think of a case where that would make sense to do that but I'll change the code just in case.
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.
I'm sure I could find a number of places in the library where we call expensive functions inside an Assert
. It may just be the style in the CUDA code to never do that and to always only use the macro to check return codes -- that's what we do for the PETSc wrappers, for example, and also for MPI. If that's the case, that's ok with me then, but it's worth remembering that one can, in principle, call an expensive function inside an assertion and that that should not happen in release mode.
include/deal.II/base/exceptions.h
Outdated
#define AssertCusparse(error_code) Assert(error_code == CUSPARSE_STATUS_SUCCESS, \ | ||
dealii::ExcCusparseError( \ | ||
dealii::deal_II_exceptions::internals:: \ | ||
get_cusparse_error_string(error_code))) | ||
#else | ||
#define AssertCusparse(error_code) { (void) error_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.
same here, of course
This compiles and passes all tests for me. |
This should get ride of a lot of warnings from the cuda build in release mode