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.

Sign up for GitHub

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

Jump to bottom

Feat 565 remove redundant thrust dialect conditional #566

Merged
merged 34 commits into from
Mar 20, 2024
Merged

Feat 565 remove redundant thrust dialect conditional #566

merged 34 commits into from
Mar 20, 2024

Conversation

ZelboK
Copy link
Contributor

@ZelboK ZelboK commented Oct 14, 2023

Currently there is wide use of THRUST_CPP_DIALECT >= 2011 through out the codebase. This conditional always leads to true, so it can be safely removed.

@ZelboK ZelboK requested review from a team as code owners October 14, 2023 21:18
@ZelboK ZelboK requested review from gevtushenko and griwes and removed request for a team October 14, 2023 21:18
@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 14, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ZelboK ZelboK changed the title Feat 565 remove redundant thrust dialect conditional [DRAFT] Feat 565 remove redundant thrust dialect conditional Oct 14, 2023
@ZelboK ZelboK changed the title [DRAFT] Feat 565 remove redundant thrust dialect conditional Feat 565 remove redundant thrust dialect conditional Oct 14, 2023
miscco
miscco reviewed Oct 16, 2023
View reviewed changes
Copy link
Collaborator

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Great work 😸
I had some minor comments about replacing now unneeded macros.

thrust/internal/benchmark/bench.cu Outdated Show resolved Hide resolved
thrust/thrust/detail/alignment.h Outdated Show resolved Hide resolved
thrust/thrust/type_traits/is_trivially_relocatable.h Outdated Show resolved Hide resolved
thrust/thrust/zip_function.h Outdated Show resolved Hide resolved
@miscco
Copy link
Collaborator

miscco commented Oct 16, 2023

/ok to test

ZelboK and others added 6 commits October 16, 2023 15:03
@miscco
Copy link
Collaborator

miscco commented Oct 17, 2023

/ok to test

@miscco
Copy link
Collaborator

miscco commented Oct 17, 2023

I believe you need to replace those macros like THRUST_ALIGNOF

@ZelboK ZelboK requested a review from a team as a code owner October 22, 2023 02:38
@miscco
Copy link
Collaborator

miscco commented Oct 22, 2023

I believe something went wrong here and you somehow added your sccache cache to the repository

@ZelboK
Merge branch 'feat-565-remove-thrust-conditional' of github.com:Zelbo…
aaa2f2e 
…K/cccl into feat-565-remove-thrust-conditional
@miscco
Copy link
Collaborator

miscco commented Oct 24, 2023

/ok to test

@jrhemstad
Copy link
Collaborator

/okay to test

@miscco
Copy link
Collaborator

miscco commented Nov 8, 2023

/ok to test

@miscco
Copy link
Collaborator

miscco commented Nov 9, 2023

@ZelboK thanks a lot for working on this. This is definitely ready to merge. That said, we are currently in the process of releasing 2.3.x and we would like to wait for that to be done, as this PR could potentially create a lot of merge conflicts

ZelboK
ZelboK commented Feb 19, 2024
View reviewed changes
thrust/thrust/zip_function.h
@@ -18,8 +18,7 @@
#endif // no system header
#include <thrust/detail/cpp11_required.h>
#include <thrust/detail/modern_gcc_required.h>

#if _CCCL_STD_VER >= 2011 && !defined(THRUST_LEGACY_GCC)
#if !defined(THRUST_LEGACY_GCC)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if im wrong but is there another macro for this one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, but that is fine

@ZelboK
Copy link
Contributor Author

ZelboK commented Feb 19, 2024 •
edited
Loading

@jrhemstad I looked over teh PR, I think this should be ready. Could use another set of 👀

Edit: I don't know why, but the UI on github isn't showing all my commits. I pushed my changes and it's missing a commit.

main...ZelboK:cccl:feat-565-remove-thrust-conditional

They are shown there though.

@miscco
Copy link
Collaborator

miscco commented Mar 13, 2024

/ok to test

@miscco miscco enabled auto-merge (squash) March 13, 2024 13:43
@ZelboK
Copy link
Contributor Author

ZelboK commented Mar 13, 2024

@miscco Do I need to do anything from my end? Should be good to go?

@miscco
Copy link
Collaborator

miscco commented Mar 13, 2024

@ZelboK we just have to wait for Ci to pass

auto-merge was automatically disabled March 14, 2024 07:05

Pull Request is not mergeable

@miscco miscco merged commit 117ebda into NVIDIA:main Mar 20, 2024
586 checks passed
@miscco
Copy link
Collaborator

miscco commented Mar 20, 2024

Thanks a lot for keeping the codebase up to date 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miscco miscco miscco approved these changes

@gevtushenko gevtushenko Awaiting requested review from gevtushenko gevtushenko is a code owner automatically assigned from NVIDIA/cccl-thrust-codeowners

@griwes griwes Awaiting requested review from griwes griwes is a code owner automatically assigned from NVIDIA/cccl-codeowners

Assignees
No one assigned
Labels
None yet
Projects
CCCL
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ZelboK @miscco @jrhemstad