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

Error macros now have GPU variant. #4465

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

kjetilly
Copy link
Contributor

@kjetilly kjetilly commented Feb 6, 2025

This will allow us to keep code such as

OPM_THROW(std::exception, "Message");

also in the GPU kernels. The alternative would be to wrap every such throw-statement in the following manner:

#if OPM_IS_INSIDE_DEVICE_FUNCTION
assert(false);
#else
OPM_THROW(std::exception, "Message");
#endif

Note that while it is possible to supply the message to assert as well, this is not really practically feasible in all the places the throw macro is used, and it is probably not needed either.

While not necessary for this to build, I have also included unit tests for the updated macros in the opm-simulators PR 5962

@kjetilly
Copy link
Contributor Author

kjetilly commented Feb 6, 2025

jenkins build this opm-simulators=5962 please

1 similar comment
@kjetilly
Copy link
Contributor Author

kjetilly commented Feb 6, 2025

jenkins build this opm-simulators=5962 please

@kjetilly kjetilly requested a review from akva2 February 7, 2025 06:47
Copy link
Member

@akva2 akva2 left a comment

Choose a reason for hiding this comment

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

I like it. Of course those writing gpu code has to be aware of the interface here as it's not all that obvious how things are tied together. (ie you have to re-check on cpu side), but at least those who are writing cpu code and are unaware they are being exploited by the gpu guys don't have to do anything special. One small request which I think makes things more readable.

@kjetilly
Copy link
Contributor Author

kjetilly commented Feb 7, 2025

jenkins build this opm-simulators=5962 please

@kjetilly
Copy link
Contributor Author

kjetilly commented Feb 7, 2025

Jenkins build this please

@bska
Copy link
Member

bska commented Feb 7, 2025

Just out of curiosity: Does assert() on GPUs have the same semantics as on the CPU–i.e., if someone #defines NDEBUG, then the assert() is just removed?

@kjetilly
Copy link
Contributor Author

kjetilly commented Feb 7, 2025

Just out of curiosity: Does assert() on GPUs have the same semantics as on the CPU–i.e., if someone #defines NDEBUG, then the assert() is just removed?

Yes, that is correct. You can see this effect in the following Compiler Explorer example that compiles since the assert statement is just empty:

#define NDEBUG 1
#include <cassert>
#include <cuda.h>
#include <cuda_runtime.h>
#include <cstdio>


// Type your code here, or load an example.
__global__ void square() {
    printf("From the kernel\n");
    assert(variable_that_does_not_exist);
}

int main(int, char**) {
    square<<<1, 1>>>();
    cudaDeviceSynchronize();
}

This is a risk, but we are also talking about code that will mostly be run in an inner loop. I can replace it with __trap() which will always abort, but I think it probably is wise to be able to compile the CUDA code in ways the compiler knows the kernel will never prematurely exit.

@bska
Copy link
Member

bska commented Feb 7, 2025

This is a risk, but we are also talking about code that will mostly be run in an inner loop. I can replace it with __trap() which will always abort, but I think it probably is wise to be able to compile the CUDA code in ways the compiler knows the kernel will never prematurely exit.

Agreed. You're the expert here, so if you're happy with this trade-off, then I have no objection.

@kjetilly
Copy link
Contributor Author

kjetilly commented Feb 7, 2025

Now, the current version does not protect against:

OPM_ERROR_IF(functionThatMustBeCalled(), "message");

I can amend this, though. Adding __trap() I am a bit more dubious towards, at least before properly benchmarking it.

@kjetilly
Copy link
Contributor Author

kjetilly commented Feb 7, 2025

Jenkins build this please

@kjetilly
Copy link
Contributor Author

Can this be merged or does it need any other changes?

@kjetilly
Copy link
Contributor Author

This PR is in principle fine to merge, BUT, we have seen that it does not work as expected on Hipcc, and would like to wait with merging until we have figured out the Hipcc problems

@kjetilly
Copy link
Contributor Author

jenkins build this opm-simulators=5962 please

1 similar comment
@multitalentloes
Copy link
Contributor

jenkins build this opm-simulators=5962 please

@kjetilly
Copy link
Contributor Author

jenkins build this hipify opm-simulators=5962 please

@multitalentloes multitalentloes merged commit 2deec42 into OPM:master Feb 21, 2025
1 check 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.

4 participants