Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Use pragma to disable execution checks in cuda::proclaim_return_type. #448

Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented May 9, 2023

Use pragma suggested by @jrhemstad to disable execution checks in cuda::proclaim_return_type. This fixes a problem with nested device lambdas. Resolves #447.

I'm not familiar with the layout of libcudacxx so I'm not sure what kind of test is appropriate to add here.

There is a minimal reproducer at https://godbolt.org/z/oaMhrcoPv, also noted in issue #447. I tested this locally with a more complex source file from libcudf (see here) and it compiled successfully after applying this patch.

@miscco
Copy link
Collaborator

miscco commented May 9, 2023

Thanks a lot for the contribution 🎉

I am adding some cleanup because we want to centralize a macro in our __config, so that we can reduce the amount of churn and then use that consistently throughout our codebase

@bdice
Copy link
Contributor Author

bdice commented May 9, 2023

There was a GitHub outage this morning. Some commits from @miscco are not showing in this PR. I am going to open this PR from draft state, since that fixed the problem for another PR where I saw this earlier today.

@bdice bdice marked this pull request as ready for review May 9, 2023 18:36
@bdice bdice force-pushed the cuda-proclaim-return-type-nv-exec-check-disable branch from 3480817 to b1deea2 Compare May 9, 2023 18:37
@bdice
Copy link
Contributor Author

bdice commented May 9, 2023

@miscco I think you can close #450 now. Further pushes to my branch should be fixed.

@miscco miscco requested review from gevtushenko and wmaxey May 9, 2023 19:32
.upstream-tests/test/support/cuda_space_selector.h Outdated Show resolved Hide resolved
.upstream-tests/test/support/cuda_space_selector.h Outdated Show resolved Hide resolved
include/cuda/functional Outdated Show resolved Hide resolved
@@ -394,6 +400,7 @@ __invoke(_Fp&& __f, _A0&& __a0)

// bullet 7

_LIBCUDACXX_DISABLE_EXEC_CHECK
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be aware, that this will disable execution checks for most of the library.

I am all in favor for that, but it will lead to bugs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about the potential for this problem. But I'm not sure what can be done about it... 🤔

Copy link
Collaborator

@jrhemstad jrhemstad May 9, 2023

Choose a reason for hiding this comment

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

Unavoidable I'm afraid. This has been true in Thrust for time immemorial. It's nearly impossible to have generic C++ code without heavy use of this pragma.

@miscco miscco added the testing: internal ci passed Passed internal NVIDIA CI (DVS). label May 10, 2023
@miscco miscco requested a review from jrhemstad May 10, 2023 12:09
@miscco miscco force-pushed the cuda-proclaim-return-type-nv-exec-check-disable branch from b1deea2 to 6912090 Compare May 12, 2023 07:54
@miscco
Copy link
Collaborator

miscco commented May 12, 2023

@bdice Thanks a lot for the contribution!

I have rebased the PR on latest master to resolve some minor merge conflicts.

@miscco miscco merged commit e9345f3 into NVIDIA:main May 12, 2023
@bdice
Copy link
Contributor Author

bdice commented May 12, 2023

@miscco Thank you very much!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
testing: internal ci passed Passed internal NVIDIA CI (DVS).
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

proclaim_return_type and __invoke should be decorated with nv_exec_check_disable
4 participants