Ensure that {cr}begin works with types that pull in namespace std via ADL #1685
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
miscco
requested review from
wmaxey,
elstehle,
bernhardmgruber,
ericniebler and
griwes
|
@Artem-B could you verify that this fixes your issues? |
|
The obvious wrinkle here is that those types wont find |
miscco
force-pushed
the
avoid_ambiguous_std_overloads
branch
from
3206dce
to
65627a6
Compare
|
I also added a workaround for |
miscco
force-pushed
the
avoid_ambiguous_std_overloads
branch
from
91b2cb4
to
a71699e
Compare
1 task
1 task
|
The patch appears to work, but I can only test it on reduced reproducers, and not on the rest of our code. Would it be possible to back-port the patch to |
miscco
force-pushed
the
avoid_ambiguous_std_overloads
branch
3 times, most recently
from
f28c30b
to
9a25ad6
Compare
|
@Artem-B I have pushed backported fixes here https://github.com/miscco/cccl/tree/backport_adl_fixes |
|
Only merged pull requests can be backported. |
miscco
force-pushed
the
avoid_ambiguous_std_overloads
branch
5 times, most recently
from
4172fe6
to
d9b5426
Compare
|
I love how fixing ambiguities with begin and friend brings up errors like:
|
Thank you. It does fix the issues I had and so far I do not see any new failures. There are a handful of cases where we ended up generating some local memory loads/stores, but that will need a closer look to see if I need to bump compiler thresholds, or if cccl may need an explicit loop unroll or inline somewhere. |
|
It may have broken one test: |
Oh it broke a lot more, that I am currently investigating. That test is absolutely fine, as it verified that we do not accept ambiguous swap. That would also apply to |
miscco
force-pushed
the
avoid_ambiguous_std_overloads
branch
from
d21ea26
to
c5cbfd2
Compare
miscco
force-pushed
the
avoid_ambiguous_std_overloads
branch
4 times, most recently
from
eb90065
to
fe7a126
Compare
🟨 CI Results [ Failed: 4 | Passed: 298 | Total: 302 ]
|
| # | Runner |
|---|---|
| 232 | linux-amd64-cpu16 |
| 28 | linux-amd64-gpu-v100-latest-1 |
| 24 | linux-arm64-cpu16 |
| 18 | windows-amd64-cpu16 |
👃 Inspect Changes
Modifications in project?
| Project | |
|---|---|
| CCCL Infrastructure | |
| +/- | libcu++ |
| +/- | CUB |
| +/- | Thrust |
Modifications in project or dependencies?
| Project | |
|---|---|
| CCCL Infrastructure | |
| +/- | libcu++ |
| +/- | CUB |
| +/- | Thrust |
… ADL Those types must be host only so we can safely SFINAE away our own free functions that would be ambiguous
libcu++ has issues with old compilers
Fixes [BUG]: libcudacxx clashes w/ libc++: ambiguous overload resolution for `__swallow` NVIDIA#1678
miscco
force-pushed
the
avoid_ambiguous_std_overloads
branch
2 times, most recently
from
2d75e7e
to
1ba4a91
Compare
miscco
force-pushed
the
avoid_ambiguous_std_overloads
branch
from
1ba4a91
to
5c4b22e
Compare
🟨 CI Results [ Failed: 1 | Passed: 301 | Total: 302 ]
|
| # | Runner |
|---|---|
| 232 | linux-amd64-cpu16 |
| 28 | linux-amd64-gpu-v100-latest-1 |
| 24 | linux-arm64-cpu16 |
| 18 | windows-amd64-cpu16 |
👃 Inspect Changes
Modifications in project?
| Project | |
|---|---|
| CCCL Infrastructure | |
| +/- | libcu++ |
| +/- | CUB |
| +/- | Thrust |
| CUDA Experimental |
Modifications in project or dependencies?
| Project | |
|---|---|
| CCCL Infrastructure | |
| +/- | libcu++ |
| +/- | CUB |
| +/- | Thrust |
| +/- | CUDA Experimental |
🟩 CI Results [ Failed: 0 | Passed: 302 | Total: 302 ]
|
| # | Runner |
|---|---|
| 232 | linux-amd64-cpu16 |
| 28 | linux-amd64-gpu-v100-latest-1 |
| 24 | linux-arm64-cpu16 |
| 18 | windows-amd64-cpu16 |
👃 Inspect Changes
Modifications in project?
| Project | |
|---|---|
| CCCL Infrastructure | |
| +/- | libcu++ |
| +/- | CUB |
| +/- | Thrust |
| CUDA Experimental |
Modifications in project or dependencies?
| Project | |
|---|---|
| CCCL Infrastructure | |
| +/- | libcu++ |
| +/- | CUB |
| +/- | Thrust |
| +/- | CUDA Experimental |
|
Looks like CL is green again. Would you be able to update your backport branch, so I can test the current changes with v2.3.2? |
Done |
|
All tests pass on my side. Thank you for backporting the fixes! |
wmaxey
approved these changes
May 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Those types must be host only so we can safely SFINAE away our own free functions that would be ambiguous
Fixes #1679