Skip to content

Fix nvcc warning about missing return statement #235

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

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

cartoonist
Copy link
Contributor

@cartoonist cartoonist commented Mar 11, 2024

The fix is based on the accepted answer here and other answers/comments there. The builtin function __builtin_unreachable is supported since CUDA 11.3.

@greg7mdp
Copy link
Owner

Nice. I'll wait for the CI/CD to run and merge. Thanks for the fix!

@greg7mdp greg7mdp merged commit 50fa648 into greg7mdp:master Mar 11, 2024
@cartoonist
Copy link
Contributor Author

cartoonist commented Mar 12, 2024

Thank you for merging and also for the library!

Just for the record, I just realised that there are some scenarios that this PR doesn't fix the issue: for example when using GCC, __has_builtin has been introduced in GCC 10. So for 4.5 <= GCCs < 10, which have __builtin_unreachable but not __has_builtin, the macro is replace by noop causing nvcc throws the warning.

I don't know a better way for checking whether __builtin_unreachable is available or not to cover those scenarios. So, I personally opted for using newer versions of gcc.

@greg7mdp
Copy link
Owner

greg7mdp commented Mar 12, 2024

To avoid the issue altogether, we could probably have done:

uint32_t TrailingZeros(T x) {
    uint32_t res;
    PHMAP_IF_CONSTEXPR(sizeof(T) == 8)
        res = base_internal::CountTrailingZerosNonZero64(static_cast<uint64_t>(x));
    else
        res = base_internal::CountTrailingZerosNonZero32(static_cast<uint32_t>(x));
    return res;
}

@cartoonist
Copy link
Contributor Author

I think that would do the trick.

@greg7mdp
Copy link
Owner

I'll do this then (maybe tomorrow).

@cartoonist
Copy link
Contributor Author

I just checked with gcc 9.4.0 and Cuda 11.4 and it worked.

@greg7mdp
Copy link
Owner

Thanks again @cartoonist , I just updated the repo with this last change.

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