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

c-style error format #304

Closed
wants to merge 2 commits into from
Closed

Conversation

tbeltzun
Copy link

@tbeltzun tbeltzun commented Sep 7, 2023

Fix #303.

The rationale is that only printf is supported on CUDA kernels, so we switch to c-style format.

@tbeltzun tbeltzun self-assigned this Sep 7, 2023
@tbeltzun tbeltzun marked this pull request as draft September 7, 2023 14:43
Copy link
Contributor

@klevzoff klevzoff left a comment

Choose a reason for hiding this comment

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

I'm on board with using printf for kernel debugging (there really shouldn't be any other use case to print from a kernel). However I'm a little concerned about having to switch all of our error logging to printf just because of it. Just think about the amount of changes in GEOS this will cause. It's also quite cumbersome to have to specify an additional format string "%s" everywhere while already using advanced formatting via fmt.

I think it's time we had separate macros for in-kernel vs out-of-kernel logging. The rule should be that any function/lambda marked as DEVICE or HOST_DEVICE is only allowed to use the former. Enforced (hopefully) via a CUDA CI build with debugging enabled.

"***** Block: [%u, %u, %u]\n" \
"***** Thread: [%u, %u, %u]\n" \
"***** Controlling expression (should be false): " STRINGIZE( EXP ) "\n", \
"***** MSG: " STRINGIZE( MSG )"\n\n" \
Copy link
Contributor

Choose a reason for hiding this comment

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

If MSG is a quoted format string now, do you still need to STRINGIZE it?

@tbeltzun
Copy link
Author

tbeltzun commented Dec 6, 2023

@klevzoff, thanks for the comments.

This PR was a shot in the dark regarding debugging CUDA kernels: I think it just doesn't work and requires a cleaner rework from scratch based on #303.

I think I'll close this as :

  1. I don't plan working on this (postdoc time is limited);
  2. it doesn't work as is (tons of compilation warnings with CUDA).

@tbeltzun tbeltzun closed this Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework Macros.hpp for devices
2 participants